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

Reviews
Diff

- Download Diff

Information
Submitter:	Toomas Soome
Repository:	illumos-gate
Branch:
Bugs:	11587
Depends On:
Commit:	6c4b798...
Reviewers
Groups:	general
People:

Description

../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

Testing Done

Issues

0
3
1
All Issues: 4

Description	From	Last Updated
Is it legal for dest to be UINT32_MAX? I'm just noticing that we used to check for an invalid value, ...	Robert Mustacchi	Aug. 22, 2019, 6:13 a.m.
In libi386, i386_copyin(), i386_copyout() (and i386_readin()) uses regular if (...) checks. Why the EFI implementation uses DEGUG-only assert() instead?	Gergő Mihály Doma	Sept. 3, 2019, 10:06 a.m.
Missing range check? Compared to libi386::i386_readin() function, efi_readin() passing it's arguments to read() without any checks. Why?	Gergő Mihály Doma	Sept. 3, 2019, 10:06 a.m.
Why do we cast dest in some cases but not others?	Robert Mustacchi	Sept. 12, 2019, 7:31 a.m.

Ship it!

Ship It!

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.

Toomas Soome Aug. 20, 2019, 3:02 p.m.

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.

Gergő Mihály Doma Aug. 22, 2019, 12:11 a.m.

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

Toomas Soome Aug. 22, 2019, 6:14 a.m.

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

Robert Mustacchi Aug. 23, 2019, 9:55 p.m.

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?

Toomas Soome Aug. 30, 2019, 11:19 a.m.
```
Right, I did confuse myself. Fixed now.
```

Commit:

-	22efa278de825c96ca1fbb7d326c6bbbe691dd64
+	1ee63726798eb22acf2ce050916cd9f4740dbd1b

Diff:

Revision 2 (+6 -6)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/loader/copy.c

Commit:

-	1ee63726798eb22acf2ce050916cd9f4740dbd1b
+	7ed2e4d5602cfc29c2aabb608a262b264419de2f

Diff:

Revision 3 (+6 -6)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/loader/copy.c

Ship it!

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?

Change Summary:

add overflow check

Commit:

-	7ed2e4d5602cfc29c2aabb608a262b264419de2f
+	f7f69ed60ede5c5e4528e1c031506f243b78f196

Diff:

Revision 4 (+6 -6)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/loader/copy.c

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?

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?

Commit:

-	f7f69ed60ede5c5e4528e1c031506f243b78f196
+	29ae40f85e0b6e4f1a3de3bf2598646df77ec45d

Diff:

Revision 5 (+24 -12)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/loader/copy.c

Change Summary:

I think I had the condition reversed:D

Commit:

-	29ae40f85e0b6e4f1a3de3bf2598646df77ec45d
+	acfafaaf343945367feaae16ecda6b1d687af546

Diff:

Revision 6 (+24 -12)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/loader/copy.c

Change Summary:

for 32-bit build, we need to cast to 64-bit data type to have meaningful test.

Commit:

-	acfafaaf343945367feaae16ecda6b1d687af546
+	02e246f653ab9bdb130d47c25557205bb55e00d5

Diff:

Revision 7 (+24 -12)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/loader/copy.c

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?

Toomas Soome Sept. 9, 2019, 4:31 p.m.

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.

Robert Mustacchi Sept. 10, 2019, 8:56 p.m.

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.

Commit:

-	02e246f653ab9bdb130d47c25557205bb55e00d5
+	6c4b7989f2a43b830c120bd263c70c8e23e1011c

Diff:

Revision 8 (+24 -12)

Show changes

	usr/src/boot/Makefile.version
	usr/src/boot/sys/boot/efi/loader/copy.c

Ship it!

Ship It!

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Issues

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Status: Closed (submitted)