| Message ID | 384e9535556e09169a16c4d6e28e419322ecf26f.1455553470.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Mon, Feb 15, 2016 at 08:32:33AM -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> > --- > > Changes from v3: None Did you have any Acked-by's thus far? > > Changes from v2: > - dmi_walk failure is no longer fatal (Jean) > > 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 | 74 +++++++++++++++++++++++++---------------- > 1 file changed, 46 insertions(+), 28 deletions(-) > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index 368e193c2741..d6ae69e0a787 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c ... > @@ -379,11 +396,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) > > keymap[hotkey_num].type = KE_END; > > - return keymap; > + results->keymap = keymap; This appears to change this function not to return anything, while the declaration appears to still expect a "const struct key_entry *". Is my visual diff parser broken?
On Tue, Feb 16, 2016 at 10:37:09PM -0800, Darren Hart wrote: > On Mon, Feb 15, 2016 at 08:32:33AM -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> > > --- > > > > Changes from v3: None > > Did you have any Acked-by's thus far? > > > > > Changes from v2: > > - dmi_walk failure is no longer fatal (Jean) > > > > 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 | 74 +++++++++++++++++++++++++---------------- > > 1 file changed, 46 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > > index 368e193c2741..d6ae69e0a787 100644 > > --- a/drivers/platform/x86/dell-wmi.c > > +++ b/drivers/platform/x86/dell-wmi.c > > ... > > > @@ -379,11 +396,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) > > > > keymap[hotkey_num].type = KE_END; > > > > - return keymap; > > + results->keymap = keymap; > > This appears to change this function not to return anything, while the > declaration appears to still expect a "const struct key_entry *". Is my visual > diff parser broken? Duh, nevermind. The context matching the before-renamed and new signature function threw me initially. Apologies.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index 368e193c2741..d6ae69e0a787 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -120,7 +120,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 = { @@ -337,20 +340,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 < @@ -379,11 +396,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(); @@ -394,20 +412,31 @@ 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; - } + if (dmi_walk(handle_dmi_entry, &dmi_results)) { + /* + * Historically, dell-wmi ignored dmi_walk errors. A failure + * is certainly surprising, but it probably just indicates + * a very old laptop. + */ + pr_warn("no DMI; using the old-style hotkey interface\n"); + } + + 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, keymap, NULL); + 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); @@ -434,15 +463,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); - } -} - /* * Descriptor buffer is 128 byte long and contains: * @@ -524,8 +544,6 @@ static int __init dell_wmi_init(void) if (err) return err; - dmi_walk(find_hk_type, NULL); - err = dell_wmi_input_setup(); if (err) return 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> --- Changes from v3: None Changes from v2: - dmi_walk failure is no longer fatal (Jean) 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 | 74 +++++++++++++++++++++++++---------------- 1 file changed, 46 insertions(+), 28 deletions(-)