Skip to contentSkip to conversation

firmware: dmi: Check DMI structure length

PATCH In review Jean Delvare 3 participants 8 messages Jun 1, 2017 – Jun 3, 2017 Dmi/smbios support

lore
Share & tools
lore

Jean Delvare

author
#0 lore raw

PATCH 1/1 firmware: dmi: Check DMI structure length

Before accessing DMI data to record it for later, we should ensure that the DMI structures are large enough to contain the data in question.

Signed-off-by Jean Delvare <[email protected]>
Cc Dmitry Torokhov <[email protected]>
Cc Mika Westerberg <[email protected]>
Cc Andy Shevchenko <[email protected]>
Cc Linus Walleij <[email protected]>
---
 drivers/firmware/dmi_scan.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)
drivers/firmware/dmi_scan.c +16 -7
@@ -178,7 +178,7 @@ static void __init dmi_save_ident(const
 	const char *d = (const char *) dm;
 	const char *p;
 
-	if (dmi_ident[slot])
+	if (dmi_ident[slot] || dm->length <= string)
 		return;
 
 	p = dmi_string(dm, d[string]);

@@ -191,13 +191,14 @@ static void __init dmi_save_ident(const
 static void __init dmi_save_uuid(const struct dmi_header *dm, int slot,
 		int index)
 {
-	const u8 *d = (u8 *) dm + index;
+	const u8 *d;
 	char *s;
 	int is_ff = 1, is_00 = 1, i;
 
-	if (dmi_ident[slot])
+	if (dmi_ident[slot] || dm->length <= index + 16)
 		return;
 
+	d = (u8 *) dm + index;
 	for (i = 0; i < 16 && (is_ff || is_00); i++) {
 		if (d[i] != 0x00)
 			is_00 = 0;

@@ -228,16 +229,17 @@ static void __init dmi_save_uuid(const s
 static void __init dmi_save_type(const struct dmi_header *dm, int slot,
 		int index)
 {
-	const u8 *d = (u8 *) dm + index;
+	const u8 *d;
 	char *s;
 
-	if (dmi_ident[slot])
+	if (dmi_ident[slot] || dm->length <= index)
 		return;
 
 	s = dmi_alloc(4);
 	if (!s)
 		return;
 
+	d = (u8 *) dm + index;
 	sprintf(s, "%u", *d & 0x7F);
 	dmi_ident[slot] = s;
 }

@@ -278,9 +280,13 @@ static void __init dmi_save_devices(cons
 
 static void __init dmi_save_oem_strings_devices(const struct dmi_header *dm)
 {
-	int i, count = *(u8 *)(dm + 1);
+	int i, count;
 	struct dmi_device *dev;
 
+	if (dm->length < 0x05)
+		return;
+
+	count = *(u8 *)(dm + 1);
 	for (i = 1; i <= count; i++) {
 		const char *devname = dmi_string(dm, i);
 

@@ -353,6 +359,9 @@ static void __init dmi_save_extended_dev
 	const char *name;
 	const u8 *d = (u8 *)dm;
 
+	if (dm->length < 0x0B)
+		return;
+
 	/* Skip disabled device */
 	if ((d[0x5] & 0x80) == 0)
 		return;

@@ -387,7 +396,7 @@ static void __init save_mem_devices(cons
 	const char *d = (const char *)dm;
 	static int nr;
 
-	if (dm->type != DMI_ENTRY_MEM_DEVICE)
+	if (dm->type != DMI_ENTRY_MEM_DEVICE || dm->length < 0x12)
 		return;
 	if (nr >= dmi_memdev_nr) {
 		pr_warn(FW_BUG "Too many DIMM entries in SMBIOS table\n");
Signature · 2 lines
Jean Delvare
SUSE L3 Support
#1 lore raw

drivers/firmware/dmi_scan.c static void __init dmi_save_ident(const · line 201

Quoted 8 lines · Jean Delvare

Before accessing DMI data to record it for later, we should ensure that the DMI structures are large enough to contain the data in question.

drivers/firmware/dmi_scan.c +2 -1
-       const u8 *d = (u8 *) dm + index;
+       const u8 *d;
 
+       d = (u8 *) dm + index;

I think you may leave this as is and make it compiler's burden to optimize.

drivers/firmware/dmi_scan.c static void __init dmi_save_ident(const · line 201

Quoted 4 lines
drivers/firmware/dmi_scan.c +2 -1
-       const u8 *d = (u8 *) dm + index;
+       const u8 *d;
 
+       d = (u8 *) dm + index;

Ditto.

drivers/firmware/dmi_scan.c static void __init dmi_save_devices(cons · line 289

Quoted 4 lines
drivers/firmware/dmi_scan.c +2 -1
-       int i, count = *(u8 *)(dm + 1);
+       int i, count;
 
+       count = *(u8 *)(dm + 1);

Ditto.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#2 lore raw

Hi Andy,

Thanks for the review.

Quoted 11 lines · Andy Shevchenko

drivers/firmware/dmi_scan.c static void __init dmi_save_ident(const · line 201

Quoted 8 lines · Jean Delvare

Before accessing DMI data to record it for later, we should ensure that the DMI structures are large enough to contain the data in question.

drivers/firmware/dmi_scan.c +2 -1
-       const u8 *d = (u8 *) dm + index;
+       const u8 *d;  
 
+       d = (u8 *) dm + index;

I think you may leave this as is and make it compiler's burden to optimize.

Is there any benefit except making the patch smaller?

Quoted 5 lines

drivers/firmware/dmi_scan.c static void __init dmi_save_ident(const · line 201

Quoted 4 lines
drivers/firmware/dmi_scan.c +2 -1
-       const u8 *d = (u8 *) dm + index;
+       const u8 *d;  
 
+       d = (u8 *) dm + index;
Quoted 8 lines

Ditto.

drivers/firmware/dmi_scan.c static void __init dmi_save_devices(cons · line 289

Quoted 4 lines
drivers/firmware/dmi_scan.c +2 -1
-       int i, count = *(u8 *)(dm + 1);
+       int i, count;  
 
+       count = *(u8 *)(dm + 1);

Ditto.

I would expect a static code analyzer to complain about at least the last one. Dereferencing a pointer before checking its validity is bad.

I'm not a big fan of counting of compiler optimizations to make the code right.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#3 lore raw
Quoted 11 lines · Jean Delvare
Quoted 8 lines · Andy Shevchenko

drivers/firmware/dmi_scan.c static void __init dmi_save_ident(const · line 201

Quoted 4 lines · Jean Delvare
drivers/firmware/dmi_scan.c +2 -1
-       const u8 *d = (u8 *) dm + index;
+       const u8 *d;
 
+       d = (u8 *) dm + index;

I think you may leave this as is and make it compiler's burden to optimize.

Is there any benefit except making the patch smaller?

Your commit message should answer to the question why and what. You didn't put it there.
Moreover, the change above per se doesn't belong to this — one logical change per patch.

Quoted 9 lines
Quoted 6 lines

drivers/firmware/dmi_scan.c static void __init dmi_save_devices(cons · line 289

Quoted 4 lines
drivers/firmware/dmi_scan.c +2 -1
-       int i, count = *(u8 *)(dm + 1);
+       int i, count;
 
+       count = *(u8 *)(dm + 1);

Ditto.

I would expect a static code analyzer to complain about at least the last one. Dereferencing a pointer before checking its validity is bad.

I would agree on this (I actually noticed it after sent a previous email)...

Quoted 2 lines

I'm not a big fan of counting of compiler optimizations to make the code right.

...but see above regarding two other changes and missed explanation in commit message for this as well.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#4 lore raw
Quoted 9 lines · Andy Shevchenko
Quoted 8 lines · Jean Delvare
Quoted 7 lines · Andy Shevchenko
Quoted 4 lines · Jean Delvare
drivers/firmware/dmi_scan.c +2 -1
-       const u8 *d = (u8 *) dm + index;
+       const u8 *d;  
 
+       d = (u8 *) dm + index;
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

I think you may leave this as is and make it compiler's burden to optimize.

Is there any benefit except making the patch smaller?

Your commit message should answer to the question why and what. You didn't put it there.
Moreover, the change above per se doesn't belong to this — one logical change per patch.

I'm confused. These changes totally belong to this patch. They belong so much to it, that's the very reason why they are not described separately in the commit message.

The purpose of the patch is to check that the records are large enough to contain the fields we need to access. Setting a pointer beyond the end of the record _before_ performing that check makes no sense.

I did not include these changes as performance optimizations, I included them because they make the code conceptually correct. It's even clearer for the last instance, as we are dereferencing the pointer immediately, but in my opinion, even setting a pointer to a location which may not exist is equally wrong and confusing for the reader. That's why I moved that code after the length checks.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#5 lore raw
Quoted 27 lines · Jean Delvare
Quoted 9 lines · Andy Shevchenko
Quoted 8 lines · Jean Delvare
Quoted 7 lines · Andy Shevchenko
Quoted 4 lines · Jean Delvare
drivers/firmware/dmi_scan.c +2 -1
-       const u8 *d = (u8 *) dm + index;
+       const u8 *d;
 
+       d = (u8 *) dm + index;
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

I think you may leave this as is and make it compiler's burden to optimize.

Is there any benefit except making the patch smaller?

Your commit message should answer to the question why and what. You didn't put it there.
Moreover, the change above per se doesn't belong to this — one logical change per patch.

I'm confused. These changes totally belong to this patch. They belong so much to it, that's the very reason why they are not described separately in the commit message.

The purpose of the patch is to check that the records are large enough to contain the fields we need to access. Setting a pointer beyond the end of the record _before_ performing that check makes no sense.

Quoted 6 lines

I did not include these changes as performance optimizations, I included them because they make the code conceptually correct. It's even clearer for the last instance, as we are dereferencing the pointer immediately, but in my opinion, even setting a pointer to a location which may not exist is equally wrong and confusing for the reader. That's why I moved that code after the length checks.

You are talking here explicitly about third case which I agreed on.

The two first ones are not the same.
You didn't dereference them before check since your check is not against pointer.

So, basically it means you are checking pointer _indirectly_.

I think we already spent too much time on this one.

If you wish to leave your changes, update commit message accordingly.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#6 lore raw
Quoted 22 lines · Andy Shevchenko
Quoted 14 lines · Jean Delvare
Quoted 4 lines · Andy Shevchenko

Your commit message should answer to the question why and what. You didn't put it there.
Moreover, the change above per se doesn't belong to this — one logical change per patch.

I'm confused. These changes totally belong to this patch. They belong so much to it, that's the very reason why they are not described separately in the commit message.

The purpose of the patch is to check that the records are large enough to contain the fields we need to access. Setting a pointer beyond the end of the record _before_ performing that check makes no sense.

Quoted 6 lines

I did not include these changes as performance optimizations, I included them because they make the code conceptually correct. It's even clearer for the last instance, as we are dereferencing the pointer immediately, but in my opinion, even setting a pointer to a location which may not exist is equally wrong and confusing for the reader. That's why I moved that code after the length checks.

Quoted 7 lines

You are talking here explicitly about third case which I agreed on.

The two first ones are not the same.
You didn't dereference them before check since your check is not against pointer.

So, basically it means you are checking pointer _indirectly_.

Correct.

Quoted 1 line

I think we already spent too much time on this one.

Agreed.

Quoted 1 line

If you wish to leave your changes, update commit message accordingly.

No.

Signature · 2 lines
Jean Delvare
SUSE L3 Support