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]

Message-ID: <CAHp75Vd1-9p4mZRie9cGG=giPt_2S-RfTfEo=fYRxYEOuv_EFw@mail.gmail.com>
Date:   Fri, 2 Jun 2017 21:45:37 +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 Fri, Jun 2, 2017 at 9:40 PM, Jean Delvare <jdelvare@...e.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@...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.
>
> 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