| Message ID | eb210629e07df3a6295a2b3b370aff87625cfbef.1453150613.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Andy, On Mon, 18 Jan 2016 12:59:38 -0800, Andy Lutomirski wrote: > The dmi_walk function maps the DMI table, walks it, and unmaps it. > This means that the dell_bios_hotkey_table that find_hk_type stores > points to unmapped memory by the time it gets read. > > I've been able to trigger crashes caused by the stale pointer a > couple of times, but never on a stock kernel. > > Fix it by generating the keymap in the dmi_walk callback instead of > storing a pointer. > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > > Notes: > Changes from v1: > - Rename handle_dmi_table to handle_dmi_entry (Jean) > - Remove useless assignment to results->err (Jean) > > drivers/platform/x86/dell-wmi.c | 68 +++++++++++++++++++++++++---------------- > 1 file changed, 41 insertions(+), 27 deletions(-) > (...) > @@ -431,7 +446,6 @@ static int __init dell_wmi_init(void) > return -ENODEV; > } > > - dmi_walk(find_hk_type, NULL); > Please also delete the following blank line, to avoid two consecutive blank lines. > err = dell_wmi_input_setup(); > if (err) Other than this, it looks good to me, so after fixing the above, you can add: Reviewed-by: Jean Delvare <jdelvare@suse.de>
Hi Andy, One more thing I just noticed... On Mon, 18 Jan 2016 12:59:38 -0800, Andy Lutomirski wrote: > @@ -372,20 +390,26 @@ static int __init dell_wmi_input_setup(void) > dell_wmi_input_dev->phys = "wmi/input0"; > dell_wmi_input_dev->id.bustype = BUS_HOST; > > - if (dell_new_hk_type) { > - const struct key_entry *keymap = dell_wmi_prepare_new_keymap(); > - if (!keymap) { > - err = -ENOMEM; > - goto err_free_dev; > - } > + err = dmi_walk(handle_dmi_entry, &dmi_results); > + if (err) > + goto err_free_dev; > (...) > @@ -431,7 +446,6 @@ static int __init dell_wmi_init(void) > return -ENODEV; > } > > - dmi_walk(find_hk_type, NULL); > > err = dell_wmi_input_setup(); > if (err) Before, in the absence of DMI support, the driver would load and assume the old hotkey interface. After your change, the driver will fail to load if DMI support is missing. This could, in theory, cause a regression (although I would be very surprised if even the oldest supported models don't implement DMI.) If you want this patch to make it into stable, you probably should stick to the original behavior and move that change (if you still believe it is a good idea) to a separate patch, and at the same time make DELL_WMI either select or depend on DMI. Many other platform/x86 drivers should do the same, BTW, but that's a separate topic.
On Tue, Jan 19, 2016 at 3:04 AM, Jean Delvare <jdelvare@suse.de> wrote: > Hi Andy, > > One more thing I just noticed... > > On Mon, 18 Jan 2016 12:59:38 -0800, Andy Lutomirski wrote: >> @@ -372,20 +390,26 @@ static int __init dell_wmi_input_setup(void) >> dell_wmi_input_dev->phys = "wmi/input0"; >> dell_wmi_input_dev->id.bustype = BUS_HOST; >> >> - if (dell_new_hk_type) { >> - const struct key_entry *keymap = dell_wmi_prepare_new_keymap(); >> - if (!keymap) { >> - err = -ENOMEM; >> - goto err_free_dev; >> - } >> + err = dmi_walk(handle_dmi_entry, &dmi_results); >> + if (err) >> + goto err_free_dev; >> (...) >> @@ -431,7 +446,6 @@ static int __init dell_wmi_init(void) >> return -ENODEV; >> } >> >> - dmi_walk(find_hk_type, NULL); >> >> err = dell_wmi_input_setup(); >> if (err) > > Before, in the absence of DMI support, the driver would load and assume > the old hotkey interface. After your change, the driver will fail to > load if DMI support is missing. This could, in theory, cause a > regression (although I would be very surprised if even the oldest > supported models don't implement DMI.) I switched it from a failure to a pr_warn. > > If you want this patch to make it into stable, you probably should > stick to the original behavior and move that change (if you still > believe it is a good idea) to a separate patch, and at the same time > make DELL_WMI either select or depend on DMI. Many other platform/x86 > drivers should do the same, BTW, but that's a separate topic. > I'm going to add a patch for that to the series, without Cc: stable. I don't think it's a big enough deal to be worth -stable churn. --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 b6f193b07566..5c0d037fcd40 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -114,7 +114,10 @@ struct dell_bios_hotkey_table { }; -static const struct dell_bios_hotkey_table *dell_bios_hotkey_table; +struct dell_dmi_results { + int err; + struct key_entry *keymap; +}; /* Uninitialized entries here are KEY_RESERVED == 0. */ static const u16 bios_to_linux_keycode[256] __initconst = { @@ -315,20 +318,34 @@ static void dell_wmi_notify(u32 value, void *context) kfree(obj); } -static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) +static void __init handle_dmi_entry(const struct dmi_header *dm, + void *opaque) { - int hotkey_num = (dell_bios_hotkey_table->header.length - 4) / - sizeof(struct dell_bios_keymap_entry); + struct dell_dmi_results *results = opaque; + struct dell_bios_hotkey_table *table; struct key_entry *keymap; - int i; + int hotkey_num, i; + + if (results->err || results->keymap) + return; /* We already found the hotkey table. */ + + if (dm->type != 0xb2 || dm->length <= 6) + return; + + table = container_of(dm, struct dell_bios_hotkey_table, header); + + hotkey_num = (table->header.length - 4) / + sizeof(struct dell_bios_keymap_entry); keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL); - if (!keymap) - return NULL; + if (!keymap) { + results->err = -ENOMEM; + return; + } for (i = 0; i < hotkey_num; i++) { const struct dell_bios_keymap_entry *bios_entry = - &dell_bios_hotkey_table->keymap[i]; + &table->keymap[i]; /* Uninitialized entries are 0 aka KEY_RESERVED. */ u16 keycode = (bios_entry->keycode < @@ -357,11 +374,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) keymap[hotkey_num].type = KE_END; - return keymap; + results->keymap = keymap; } static int __init dell_wmi_input_setup(void) { + struct dell_dmi_results dmi_results = {}; int err; dell_wmi_input_dev = input_allocate_device(); @@ -372,20 +390,26 @@ static int __init dell_wmi_input_setup(void) dell_wmi_input_dev->phys = "wmi/input0"; dell_wmi_input_dev->id.bustype = BUS_HOST; - if (dell_new_hk_type) { - const struct key_entry *keymap = dell_wmi_prepare_new_keymap(); - if (!keymap) { - err = -ENOMEM; - goto err_free_dev; - } + err = dmi_walk(handle_dmi_entry, &dmi_results); + if (err) + goto err_free_dev; - err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL); + if (dmi_results.err) { + err = dmi_results.err; + goto err_free_dev; + } + + if (dmi_results.keymap) { + dell_new_hk_type = true; + + err = sparse_keymap_setup(dell_wmi_input_dev, + dmi_results.keymap, NULL); /* * Sparse keymap library makes a copy of keymap so we * don't need the original one that was allocated. */ - kfree(keymap); + kfree(dmi_results.keymap); } else { err = sparse_keymap_setup(dell_wmi_input_dev, dell_wmi_legacy_keymap, NULL); @@ -412,15 +436,6 @@ static void dell_wmi_input_destroy(void) input_unregister_device(dell_wmi_input_dev); } -static void __init find_hk_type(const struct dmi_header *dm, void *dummy) -{ - if (dm->type == 0xb2 && dm->length > 6) { - dell_new_hk_type = true; - dell_bios_hotkey_table = - container_of(dm, struct dell_bios_hotkey_table, header); - } -} - static int __init dell_wmi_init(void) { int err; @@ -431,7 +446,6 @@ static int __init dell_wmi_init(void) return -ENODEV; } - dmi_walk(find_hk_type, NULL); err = dell_wmi_input_setup(); if (err)
The dmi_walk function maps the DMI table, walks it, and unmaps it. This means that the dell_bios_hotkey_table that find_hk_type stores points to unmapped memory by the time it gets read. I've been able to trigger crashes caused by the stale pointer a couple of times, but never on a stock kernel. Fix it by generating the keymap in the dmi_walk callback instead of storing a pointer. Signed-off-by: Andy Lutomirski <luto@kernel.org> --- Notes: Changes from v1: - Rename handle_dmi_table to handle_dmi_entry (Jean) - Remove useless assignment to results->err (Jean) drivers/platform/x86/dell-wmi.c | 68 +++++++++++++++++++++++++---------------- 1 file changed, 41 insertions(+), 27 deletions(-)