On Tue, Jan 19, 2016 at 12:31 AM, Jean Delvare <jdelvare@xxxxxxx> 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@xxxxxxx>
>> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
