linux-kernel - Re: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check

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: <CALCETrXheMKeQ_s4wAopk_Jcv-CwJCVrksoKdeuPDF7tsyCpFA@mail.gmail.com>
Date:	Tue, 19 Jan 2016 10:42:05 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Jean Delvare <jdelvare@...e.de>
Cc:	Andy Lutomirski <luto@...nel.org>,
	Pali Rohár <pali.rohar@...il.com>,
	platform-driver-x86@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check

On Tue, Jan 19, 2016 at 12:31 AM, Jean Delvare <jdelvare@...e.de> wrote:
> Hi Andy,
>
> On Mon, 18 Jan 2016 12:59:39 -0800, Andy Lutomirski wrote:
>> The minimum size of the table is 4, not 6.  Replace the hard-coded
>> number with a sizeof expression.  While we're at it, repace the
>> hard-coded 4 below as well.
>>
>> Reported-by: Jean Delvare <jdelvare@...e.de>
>> Signed-off-by: Andy Lutomirski <luto@...nel.org>
>> ---
>>  drivers/platform/x86/dell-wmi.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index 5c0d037fcd40..48838942d593 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -111,7 +111,6 @@ struct dell_bios_keymap_entry {
>>  struct dell_bios_hotkey_table {
>>       struct dmi_header header;
>>       struct dell_bios_keymap_entry keymap[];
>> -
>>  };
>>
>>  struct dell_dmi_results {
>
> Nice cleanup but in general we recommend to not mix style cleanups with
> functional changes. If you want to clean up dell-wmi you could do it in
> a separate patch and maybe include the fixes suggested by checkpatch.pl
> -f.

/me sheepishly puts the newline back in.

>
>> @@ -329,12 +328,14 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
>>       if (results->err || results->keymap)
>>               return;         /* We already found the hotkey table. */
>>
>> -     if (dm->type != 0xb2 || dm->length <= 6)
>> +     if (dm->type != 0xb2 ||
>> +         dm->length <= sizeof(struct dell_bios_hotkey_table))
>>               return;
>
> I'm confused. sizeof(struct dell_bios_hotkey_table) is 4. Given that
> dm->length is guaranteed to be at least 4 per the SMBIOS specification,
> you are really only testing that dm->length != 4. Which means you are
> still accepting 5, 6 and 7, even though they would lead to hotkey_num =
> 0 below.
>
> If the purpose of this check is only to guarantee that the container_of
> below is valid then you should check for dm->length < sizeof(struct
> dell_bios_hotkey_table) (not <=.) This is still useless in practice but
> I can understand and accept it because it is conceptually correct.
>
> OTOH if the purpose of the check is to ensure that there is at least
> one hotkey, you should check for dm->length < sizeof(struct
> dell_bios_hotkey_table) + sizeof(struct dell_bios_keymap_entry)
> instead. hotkey_num could also be checked separately below but it is
> more efficient to have a single test.

I think the check is just to see if the buffer is big enough, but
maybe there's history here, and I don't want to be the old to break
ancient laptops for the sake of a cleanup.  Let me try this again.

--Andy