Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1655126 > unrolled thread

[PATCH] firmware: dmi: Check DMI structure length

Back to article view | Back to linux.kernel

Contents

  [PATCH] firmware: dmi: Check DMI structure length Jean Delvare <jdelvare@suse.de> - 2017-06-01 15:10 +0200
    Re: [PATCH] firmware: dmi: Check DMI structure length Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-01 15:20 +0200
      Re: [PATCH] firmware: dmi: Check DMI structure length Jean Delvare <jdelvare@suse.de> - 2017-06-01 16:50 +0200
        Re: [PATCH] firmware: dmi: Check DMI structure length Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-01 18:10 +0200
          Re: [PATCH] firmware: dmi: Check DMI structure length Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-02 20:50 +0200
            Re: [PATCH] firmware: dmi: Check DMI structure length Jean Delvare <jdelvare@suse.de> - 2017-06-03 23:20 +0200
          Re: [PATCH] firmware: dmi: Check DMI structure length Jean Delvare <jdelvare@suse.de> - 2017-06-02 20:50 +0200
    Re: [PATCH] firmware: dmi: Check DMI structure length Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-06-01 16:10 +0200

#1655126 — [PATCH] 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 <jdelvare@suse.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/firmware/dmi_scan.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

--- linux-4.11.orig/drivers/firmware/dmi_scan.c	2017-06-01 14:43:10.718650335 +0200
+++ linux-4.11/drivers/firmware/dmi_scan.c	2017-06-01 14:59:41.428092051 +0200
@@ -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");


-- 
Jean Delvare
SUSE L3 Support

[toc] | [next] | [standalone]

#1655139

On Thu, Jun 1, 2017 at 4:08 PM, Jean Delvare <jdelvare@suse.de> wrote:
> 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.

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

> -       const u8 *d = (u8 *) dm + index;
> +       const u8 *d;

> +       d = (u8 *) dm + index;

Ditto.

> -       int i, count = *(u8 *)(dm + 1);
> +       int i, count;

> +       count = *(u8 *)(dm + 1);

Ditto.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1655201

Hi Andy,

Thanks for the review.

On Thu, 1 Jun 2017 16:16:05 +0300, Andy Shevchenko wrote:
> On Thu, Jun 1, 2017 at 4:08 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > 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.  
> 
> > -       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?

> > -       const u8 *d = (u8 *) dm + index;
> > +       const u8 *d;  
> 
> > +       d = (u8 *) dm + index;  
> 
> Ditto.
> 
> > -       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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1655357

On Thu, Jun 1, 2017 at 5:40 PM, Jean Delvare <jdelvare@suse.de> wrote:
> On Thu, 1 Jun 2017 16:16:05 +0300, Andy Shevchenko wrote:
>> On Thu, Jun 1, 2017 at 4:08 PM, Jean Delvare <jdelvare@suse.de> wrote:

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

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

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

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1656467

On Fri, Jun 2, 2017 at 9:40 PM, Jean Delvare <jdelvare@suse.de> wrote:
> On Thu, 1 Jun 2017 19:06:36 +0300, Andy Shevchenko wrote:
>> On Thu, Jun 1, 2017 at 5:40 PM, Jean Delvare <jdelvare@suse.de> wrote:
>> > On Thu, 1 Jun 2017 16:16:05 +0300, Andy Shevchenko wrote:
>> >> On Thu, Jun 1, 2017 at 4:08 PM, Jean Delvare <jdelvare@suse.de> wrote:
>>
>> >> > -       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.
>
> 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.

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.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1656929

On Fri, 2 Jun 2017 21:45:37 +0300, Andy Shevchenko wrote:
> On Fri, Jun 2, 2017 at 9:40 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Thu, 1 Jun 2017 19:06:36 +0300, Andy Shevchenko wrote:  
> >> 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.  
> 
> 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.

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

Agreed.

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

No.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1656469

On Thu, 1 Jun 2017 19:06:36 +0300, Andy Shevchenko wrote:
> On Thu, Jun 1, 2017 at 5:40 PM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Thu, 1 Jun 2017 16:16:05 +0300, Andy Shevchenko wrote:  
> >> On Thu, Jun 1, 2017 at 4:08 PM, Jean Delvare <jdelvare@suse.de> wrote:  
> 
> >> > -       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.

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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1655163

On Thu, Jun 01, 2017 at 03:08:39PM +0200, Jean Delvare wrote:
> 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 <jdelvare@suse.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks for taking care of this!

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web