From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932975AbcASSme (ORCPT ); Tue, 19 Jan 2016 13:42:34 -0500 Received: from mail-ob0-f170.google.com ([209.85.214.170]:34106 "EHLO mail-ob0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932721AbcASSm0 (ORCPT ); Tue, 19 Jan 2016 13:42:26 -0500 MIME-Version: 1.0 In-Reply-To: <20160119093146.28aa10e1@endymion.delvare> References: <0282cf1f0c15ae9006b119dd92bfb4bad2e924a7.1453150613.git.luto@kernel.org> <20160119093146.28aa10e1@endymion.delvare> From: Andy Lutomirski Date: Tue, 19 Jan 2016 10:42:05 -0800 Message-ID: Subject: Re: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check To: Jean Delvare Cc: Andy Lutomirski , =?UTF-8?Q?Pali_Roh=C3=A1r?= , platform-driver-x86@vger.kernel.org, "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 19, 2016 at 12:31 AM, Jean Delvare 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 >> Signed-off-by: Andy Lutomirski >> --- >> 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