11587 loader.efi: comparison is always true due to limited range of data type

Review Request #2241 — Created Aug. 20, 2019 and submitted

Information
tsoome
illumos-gate
11587
6c4b798...
Reviewers
general 
../copy.c: In function 'efi_copyin':
../copy.c:209:14: error: comparison is always true due to limited range of data type [-Werror=type-limits]
  209 |  assert(dest < 0x100000000);
      |              ^
../../../../../include/assert.h:54:21: note: in definition of macro 'assert'
   54 | #define assert(e) ((e) ? (void)0 : __assert(__func__, __FILE__, \
      |                     ^
../copy.c: In function 'efi_copyout':
../copy.c:217:13: error: comparison is always true due to limited range of data type [-Werror=type-limits]
  217 |  assert(src < 0x100000000);
      |             ^
../../../../../include/assert.h:54:21: note: in definition of macro 'assert'
   54 | #define assert(e) ((e) ? (void)0 : __assert(__func__, __FILE__, \
      |                     ^
cc1: all warnings being treated as errors




 













 

 






 
 
 

  

   
  

  
    
   
  • 
    
    0
   
    • 
   
  • 
    
    0
   
    • 
   
  • 
    
    3
   
    • 
   
  • 
    
    1
   
    • 
   
  • 
    
    4
   
    • 
  

 

 

  

   
    

     Description
     From
     Last Updated
    

   
   


    


    


    


    

   
  

 












  

 








 














 





 

  

   




andy_js




   

   







  

 



 

  


   

   

    
 
andy_js


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




rm




   

   







  

 



 

  


   

   

    
 
rm


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/boot/sys/boot/efi/loader/copy.c
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Is it legal for dest to be UINT32_MAX? I'm just noticing that we used to check for an invalid value, so maybe that means it should be <=? I'm not sure if that'll still end up causing problems for the 32-bit build or not.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      yes, The reason for this limit is the fact that some systems have only mapped first 4GB of memory, so we only want to avoid going any higher.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      Maybe check the whole range, not just the start: dest + len <= UINT32_MAX ?
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Good point. of course it is still < UINT32_MAX for assert condition.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      With the len addition, why exactly is it < UINT32_MAX? If we have a 10 byte field that ends at UINT32_MAX, doesn't that correctly fit?
      
 
      

      5. 



    5. 
 
 
      
 
      
  
 
      
 
      
  
      Right, I did confuse myself. Fixed now.
      
 
      

      6. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 















 



 

  

   




tsoome




   

   

  

 



 















 





 

  

   




rm




   

   







  

 



 

  


   

   

    
 
rm


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/boot/sys/boot/efi/loader/copy.c
      


       (Diff revision 3)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
    I'm assuming we don't care about overflow possibilities in the debug assert?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 















 





 

  

   




domag02




   

   







  

 



 

  


   

   

    
 
domag02


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/boot/sys/boot/efi/loader/copy.c
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    In libi386, i386_copyin(), i386_copyout() (and i386_readin()) uses regular if (...) checks.
    
Why the EFI implementation uses DEGUG-only assert() instead?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/boot/sys/boot/efi/loader/copy.c
      


       (Diff revision 4)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Missing range check?
    

    Compared to libi386::i386_readin() function, efi_readin() passing it's arguments to read() without any checks.
    
Why?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 















 



 

  

   




tsoome




   

   

  

 



 















 



 

  

   




tsoome




   

   

  

 



 















 





 

  

   




rm




   

   







  

 



 

  


   

   

    
 
rm


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      usr/src/boot/sys/boot/efi/loader/copy.c
      


       (Diff revision 7)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    



    
 
    
  
    
   
   
    
    
     
     
     
     
     
    
    Show all issues
   
  
    
 
    

    



    
    Why do we cast dest in some cases but not others?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      The dest + len <= UINT32_MAX needs cast because of 32-bit build (in 64-bit build the vm_offset_t is 64-bit int), and with only 32-bit nusigned values, we never get above UINT32_MAX.
      

      The dest + len >= dest case (negated dest + len < dest) is simplified case of test a + b < a || a + b < b and there we do not really need to cast to 64-bit because we only need to see what happens with sum of the dest and len.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I see, so in the second if block you basically want to ensure that it's always promoted to a 64-bit comparison. I guess that makes sense, though it's a bit hard to see that in a 32-bit case it'll always be three 32-bit quantities that we're comparing and if there was overflow, it'd certainly end up being less than the max.
      
 
      

      3. 


 
    


    

 
    

    3. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 















 





 

  

   




rm




   

   







  

 



 

  


   

   

    
 
rm


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




tsoome




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...