lkml.org 
[lkml]   [2016]   [Jan]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check
On Tue, Jan 19, 2016 at 12:31 AM, Jean Delvare <jdelvare@suse.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@suse.de>
>> Signed-off-by: Andy Lutomirski <luto@kernel.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

\
 
 \ /
  Last update: 2016-01-19 20:21    [W:0.052 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site