linux-kernel - Re: [PATCH] firmware: dmi: Check DMI structure length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 1 Jun 2017 19:06:36 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jean Delvare <jdelvare@...e.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH] firmware: dmi: Check DMI structure length

On Thu, Jun 1, 2017 at 5:40 PM, Jean Delvare <jdelvare@...e.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@...e.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