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

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

tsoome
illumos-gate
11587
6c4b798...
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
  1. Ship It!
  2. 
      
rm
  1. 
      
  2. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 1)
     
     
    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. Maybe check the whole range, not just the start: dest + len <= UINT32_MAX ?

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

    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. Right, I did confuse myself. Fixed now.

  3. 
      
tsoome
tsoome
rm
  1. 
      
  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. 
      
tsoome
domag02
  1. 
      
  2. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

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

  3. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 4)
     
     
     
     
     
     

    Missing range check?

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

  4. 
      
tsoome
tsoome
tsoome
rm
  1. 
      
  2. usr/src/boot/sys/boot/efi/loader/copy.c (Diff revision 7)
     
     
    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. 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. 
      
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...