| Message ID | 0282cf1f0c15ae9006b119dd92bfb4bad2e924a7.1453150613.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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. > @@ -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. > > table = container_of(dm, struct dell_bios_hotkey_table, header); > > - hotkey_num = (table->header.length - 4) / > + hotkey_num = (table->header.length - > + sizeof(struct dell_bios_hotkey_table)) / > sizeof(struct dell_bios_keymap_entry); > > keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
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 -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 { @@ -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; table = container_of(dm, struct dell_bios_hotkey_table, header); - hotkey_num = (table->header.length - 4) / + hotkey_num = (table->header.length - + sizeof(struct dell_bios_hotkey_table)) / sizeof(struct dell_bios_keymap_entry); keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
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(-)