| Message ID | 119170b95d373bc943eb4f16818239bac9fa6c59.1451832667.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Sun, Jan 3, 2016 at 6:52 AM, Andy Lutomirski <luto@kernel.org> 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. Quick ping: has anyone had a chance to look at this? --Andy > > Cc: stable@vger.kernel.org > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > > This seems to work on my laptop. It applies to platform-drivers-x86/for-next. > > drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++++---------------- > 1 file changed, 42 insertions(+), 27 deletions(-) > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index 57402c4c394e..52db2721d7e3 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c > @@ -116,7 +116,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 = { > @@ -316,20 +319,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_table(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 < > @@ -358,11 +375,13 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) > > keymap[hotkey_num].type = KE_END; > > - return keymap; > + results->err = 0; > + 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(); > @@ -373,20 +392,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_table, &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); > @@ -413,15 +438,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; > @@ -432,7 +448,6 @@ static int __init dell_wmi_init(void) > return -ENODEV; > } > > - dmi_walk(find_hk_type, NULL); > acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor; > > err = dell_wmi_input_setup(); > -- > 2.5.0 >
On Monday 11 January 2016 13:58:20 Andy Lutomirski wrote: > On Sun, Jan 3, 2016 at 6:52 AM, Andy Lutomirski <luto@kernel.org> 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. > > Quick ping: has anyone had a chance to look at this? > > --Andy > Hi Andy, I looked at this patch, but I think some people from -mm or DMI code should look at it as it is memory problem... We also has one in dell-laptop.ko (wrong API usage) and so -mm people could know it better. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Andy Lutomirski <luto@kernel.org> > > --- > > > > This seems to work on my laptop. It applies to platform-drivers-x86/for-next. > > > > drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++++---------------- > > 1 file changed, 42 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > > index 57402c4c394e..52db2721d7e3 100644 > > --- a/drivers/platform/x86/dell-wmi.c > > +++ b/drivers/platform/x86/dell-wmi.c > > @@ -116,7 +116,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 = { > > @@ -316,20 +319,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_table(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 < > > @@ -358,11 +375,13 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) > > > > keymap[hotkey_num].type = KE_END; > > > > - return keymap; > > + results->err = 0; > > + 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(); > > @@ -373,20 +392,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_table, &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); > > @@ -413,15 +438,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; > > @@ -432,7 +448,6 @@ static int __init dell_wmi_init(void) > > return -ENODEV; > > } > > > > - dmi_walk(find_hk_type, NULL); > > acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor; > > > > err = dell_wmi_input_setup(); > > -- > > 2.5.0 > > > > >
[cc: Jean Delvare] On Tue, Jan 12, 2016 at 6:25 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Monday 11 January 2016 13:58:20 Andy Lutomirski wrote: >> On Sun, Jan 3, 2016 at 6:52 AM, Andy Lutomirski <luto@kernel.org> 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. >> >> Quick ping: has anyone had a chance to look at this? >> >> --Andy >> > > Hi Andy, I looked at this patch, but I think some people from -mm or DMI > code should look at it as it is memory problem... We also has one in > dell-laptop.ko (wrong API usage) and so -mm people could know it better. Let's ask: Jean, am I right that drivers must not store pointers to DMI tables that they find through dmi_walk? Is there any alternative interface that could be used to get a longer-lived pointer to DMI tables, or should drivers just parse them and copy out any info needed from the dmi_walk callback? There are at least two platform drivers (dell-wmi and dell-laptop) that don't play well with the current interface. This patch is intended to fix one of them. --Andy > >> > >> > Cc: stable@vger.kernel.org >> > Signed-off-by: Andy Lutomirski <luto@kernel.org> >> > --- >> > >> > This seems to work on my laptop. It applies to platform-drivers-x86/for-next. >> > >> > drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++++---------------- >> > 1 file changed, 42 insertions(+), 27 deletions(-) >> > >> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c >> > index 57402c4c394e..52db2721d7e3 100644 >> > --- a/drivers/platform/x86/dell-wmi.c >> > +++ b/drivers/platform/x86/dell-wmi.c >> > @@ -116,7 +116,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 = { >> > @@ -316,20 +319,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_table(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 < >> > @@ -358,11 +375,13 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) >> > >> > keymap[hotkey_num].type = KE_END; >> > >> > - return keymap; >> > + results->err = 0; >> > + 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(); >> > @@ -373,20 +392,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_table, &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); >> > @@ -413,15 +438,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; >> > @@ -432,7 +448,6 @@ static int __init dell_wmi_init(void) >> > return -ENODEV; >> > } >> > >> > - dmi_walk(find_hk_type, NULL); >> > acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor; >> > >> > err = dell_wmi_input_setup(); >> > -- >> > 2.5.0 >> > >> >> >> > > -- > Pali Rohár > pali.rohar@gmail.com
> > Hi Andy, I looked at this patch, but I think some people from -mm or DMI > > code should look at it as it is memory problem... We also has one in > > dell-laptop.ko (wrong API usage) and so -mm people could know it better. > There are at least two platform drivers (dell-wmi and dell-laptop) > that don't play well with the current interface. This patch is > intended to fix one of them. Pali, Andy, Could you point out the exact place where dell-laptop errs? AFAICT, it only has one use of dmi_walk(), the callback there calls parse_da_table(), which does the right thing, i.e. krealloc()'ing memory and then memcpy()'ing table contents there...
> 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. > > Cc: stable@vger.kernel.org > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > > This seems to work on my laptop. It applies to platform-drivers-x86/for-next. > > drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++++---------------- > 1 file changed, 42 insertions(+), 27 deletions(-) > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index 57402c4c394e..52db2721d7e3 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c > @@ -116,7 +116,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; Nit: keymap can be a const struct key_entry *.
> Jean, am I right that drivers must not store pointers to DMI tables > that they find through dmi_walk? Is there any alternative interface > that could be used to get a longer-lived pointer to DMI tables, or > should drivers just parse them and copy out any info needed from the > dmi_walk callback? The easiest long term solution might be to just map the dmi buffer once and keep it. It's not that huge so it's not a big address space hog. Alan -- 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
Hi Alan, On Thu, 14 Jan 2016 14:07:31 +0000, One Thousand Gnomes wrote: > > Jean, am I right that drivers must not store pointers to DMI tables > > that they find through dmi_walk? Is there any alternative interface > > that could be used to get a longer-lived pointer to DMI tables, or > > should drivers just parse them and copy out any info needed from the > > dmi_walk callback? > > The easiest long term solution might be to just map the dmi buffer once > and keep it. It's not that huge so it's not a big address space hog. Please note that SMBIOS specification version 3.0 allows for 32-bit length for DMI tables, suggesting that 64k tables were not large enough for everyone. Just saying. I have no strong opinion on the matter.
On Thu, 14 Jan 2016 10:52:04 +0100, Micha? K?pie? wrote: > > > Hi Andy, I looked at this patch, but I think some people from -mm or DMI > > > code should look at it as it is memory problem... We also has one in > > > dell-laptop.ko (wrong API usage) and so -mm people could know it better. > > > There are at least two platform drivers (dell-wmi and dell-laptop) > > that don't play well with the current interface. This patch is > > intended to fix one of them. > > Pali, Andy, > > Could you point out the exact place where dell-laptop errs? AFAICT, it > only has one use of dmi_walk(), the callback there calls > parse_da_table(), which does the right thing, i.e. krealloc()'ing memory > and then memcpy()'ing table contents there... FWIW I can't see any problem with dell-laptop either.
Hi Andy, Sorry for the late reply. On Wed, 13 Jan 2016 14:28:18 -0800, Andy Lutomirski wrote: > [cc: Jean Delvare] > > On Tue, Jan 12, 2016 at 6:25 AM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Monday 11 January 2016 13:58:20 Andy Lutomirski wrote: > >> On Sun, Jan 3, 2016 at 6:52 AM, Andy Lutomirski <luto@kernel.org> 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. > >> > >> Quick ping: has anyone had a chance to look at this? > > > > Hi Andy, I looked at this patch, but I think some people from -mm or DMI > > code should look at it as it is memory problem... We also has one in > > dell-laptop.ko (wrong API usage) and so -mm people could know it better. > > Let's ask: > > Jean, am I right that drivers must not store pointers to DMI tables > that they find through dmi_walk? Yes, you are right. > Is there any alternative interface > that could be used to get a longer-lived pointer to DMI tables, or > should drivers just parse them and copy out any info needed from the > dmi_walk callback? There is no alternative for OEM type records. Drivers are indeed expected to copy the information they need to their own buffers. > There are at least two platform drivers (dell-wmi and dell-laptop) > that don't play well with the current interface. This patch is > intended to fix one of them. Couldn't see any problem with dell-laptop.
On Fri, Jan 15, 2016 at 5:27 AM, Jean Delvare <jdelvare@suse.de> wrote: > Hi Andy, > > Sorry for the late reply. > > On Wed, 13 Jan 2016 14:28:18 -0800, Andy Lutomirski wrote: >> [cc: Jean Delvare] >> >> On Tue, Jan 12, 2016 at 6:25 AM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > On Monday 11 January 2016 13:58:20 Andy Lutomirski wrote: >> >> On Sun, Jan 3, 2016 at 6:52 AM, Andy Lutomirski <luto@kernel.org> 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. >> >> >> >> Quick ping: has anyone had a chance to look at this? >> > >> > Hi Andy, I looked at this patch, but I think some people from -mm or DMI >> > code should look at it as it is memory problem... We also has one in >> > dell-laptop.ko (wrong API usage) and so -mm people could know it better. >> >> Let's ask: >> >> Jean, am I right that drivers must not store pointers to DMI tables >> that they find through dmi_walk? > > Yes, you are right. > >> Is there any alternative interface >> that could be used to get a longer-lived pointer to DMI tables, or >> should drivers just parse them and copy out any info needed from the >> dmi_walk callback? > > There is no alternative for OEM type records. Drivers are indeed > expected to copy the information they need to their own buffers. FWIW, especially if we consider mapping it persistently, maybe we should use ioremap_prot and map it both cached and ro. Actually, switching to a cached mapping regardless of persistence could noticeably help boot times. UC accesses are very, very slow. --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
Hi Andy, Le Friday 15 January 2016 à 08:27 -0800, Andy Lutomirski a écrit : > FWIW, especially if we consider mapping it persistently, maybe we > should use ioremap_prot and map it both cached and ro. > > Actually, switching to a cached mapping regardless of persistence > could noticeably help boot times. UC accesses are very, very slow. Sorry if it is obvious for everybody else, but what does "UC accesses" mean?
On Fri, Jan 15, 2016 at 9:53 AM, Jean Delvare <jdelvare@suse.de> wrote: > Hi Andy, > > Le Friday 15 January 2016 à 08:27 -0800, Andy Lutomirski a écrit : >> FWIW, especially if we consider mapping it persistently, maybe we >> should use ioremap_prot and map it both cached and ro. >> >> Actually, switching to a cached mapping regardless of persistence >> could noticeably help boot times. UC accesses are very, very slow. > > Sorry if it is obvious for everybody else, but what does "UC accesses" > mean? > Sorry, I sometimes have my head buried too far in the CPU :) UC means uncached. ioremap, on x86, asks for an uncached mapping, so every memory access (load or store) hits main memory individually. Assuming that the spec says that whatever physical memory the DMI tables live in is permitted to be used with cached accesses, asking for the CPU cache to be permitted on those accesses will make them a whole lot faster. If that isn't safe, you could also just copy each table out of the ioremap space into normal RAM as needed using MOVNTDQA. I forget what the helper for that is called, but it basically does a fast streaming IO read and then writes to normal RAM, memcpy style. Most modern CPUs support it. --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
Hi Andy, On Sun, 3 Jan 2016 06:52:28 -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. > > Cc: stable@vger.kernel.org > Signed-off-by: Andy Lutomirski <luto@kernel.org> Overall I like the idea. > --- > > This seems to work on my laptop. It applies to platform-drivers-x86/for-next. > > drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++++---------------- > 1 file changed, 42 insertions(+), 27 deletions(-) > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index 57402c4c394e..52db2721d7e3 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c > @@ -116,7 +116,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 = { > @@ -316,20 +319,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_table(const struct dmi_header *dm, This is really handling one DMI structure, not the whole table. > + 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. */ Can this actually happen? > + > + 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); The problem is not introduced by your patch, but the length check is inconsistent. sizeof(struct dell_bios_keymap_entry) is 4. If we need at least one keymap entry then the minimum size would be 8, while the check above would accept 7. If 7 is fine (empty keymap) then 4, 5 and 6 are equally fine and the length check can be dropped. If not, the length check should be fixed. If we only care about having at least one key, then checking for hotkey_num >= is better. If we want to check for more consistency then we should check that table->header.length - 4 is a strictly positive multiple of 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 < > @@ -358,11 +375,13 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) > > keymap[hotkey_num].type = KE_END; > > - return keymap; > + results->err = 0; The check at the beginning of the function assumes that results->err was already 0 originally. > + 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(); > @@ -373,20 +392,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_table, &dmi_results); > + if (err) > + goto err_free_dev; dmi_walk() returns -1 on error, not some -E value (I take the blame for that.) So you can't return it directly to the caller, otherwise it will be incorrectly interpreted as "Operation not permitted" (-1 == -EPERM.) So you must either hard-code your own -E value here, or first fix dmi_walk() to return something sane. > > - err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL); > + if (dmi_results.err) { > + err = dmi_results.err; > + goto err_free_dev; > + } I think it would make sense to fix dmi_walk() so that it lets the decoding function return error codes. This would avoid the convoluted error code handling. Not sure why I didn't do that originally :( > + > + 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); > @@ -413,15 +438,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; > @@ -432,7 +448,6 @@ static int __init dell_wmi_init(void) > return -ENODEV; > } > > - dmi_walk(find_hk_type, NULL); > acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor; > > err = dell_wmi_input_setup(); I can't test it but it looks good overall.
On Mon, Jan 18, 2016 at 7:44 AM, Jean Delvare <jdelvare@suse.de> wrote: > Hi Andy, > > On Sun, 3 Jan 2016 06:52:28 -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. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Andy Lutomirski <luto@kernel.org> > > Overall I like the idea. > >> --- >> >> This seems to work on my laptop. It applies to platform-drivers-x86/for-next. >> >> drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++++---------------- >> 1 file changed, 42 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c >> index 57402c4c394e..52db2721d7e3 100644 >> --- a/drivers/platform/x86/dell-wmi.c >> +++ b/drivers/platform/x86/dell-wmi.c >> @@ -116,7 +116,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 = { >> @@ -316,20 +319,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_table(const struct dmi_header *dm, > > This is really handling one DMI structure, not the whole table. Renamed to handle_dmi_entry. > >> + 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. */ > > Can this actually happen? > Yes, I think, if Dell ships a laptop with two tables of type 0xB2. There's no return code that says "I'm done", so I can't just stop walking the DMI data after finding what I'm looking for. >> + >> + 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); > > The problem is not introduced by your patch, but the length check is > inconsistent. sizeof(struct dell_bios_keymap_entry) is 4. Yes, but sizeof(struct dell_bios_keymap_table) is 6. > If we need at > least one keymap entry then the minimum size would be 8, while the > check above would accept 7. If 7 is fine (empty keymap) then 4, 5 and 6 > are equally fine and the length check can be dropped. If not, the > length check should be fixed. I think the length check is correct, but the hotkey_num calculation is wrong. The table is 84 bytes on my system, which makes perfect sense: 6 bytes of header and 78 == 13*6 bytes of entries. But 84-4 is *not* a multiple of 6. It should be (table->header.length - sizeof(struct dell_bios_hotkey_table) / sizeof(struct dell_bios_hotkey_enntry), I think. I'll add another patch to fix this up. >> - return keymap; >> + results->err = 0; > > The check at the beginning of the function assumes that results->err > was already 0 originally. > Good catch. I removed that line. >> + 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(); >> @@ -373,20 +392,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_table, &dmi_results); >> + if (err) >> + goto err_free_dev; > > dmi_walk() returns -1 on error, not some -E value (I take the blame for > that.) So you can't return it directly to the caller, otherwise it will > be incorrectly interpreted as "Operation not permitted" (-1 == -EPERM.) > > So you must either hard-code your own -E value here, or first fix > dmi_walk() to return something sane. I'll submit a patch to change dmi_walk and dmi_walk_early. > >> >> - err = sparse_keymap_setup(dell_wmi_input_dev, keymap, NULL); >> + if (dmi_results.err) { >> + err = dmi_results.err; >> + goto err_free_dev; >> + } > > I think it would make sense to fix dmi_walk() so that it lets the > decoding function return error codes. This would avoid the convoluted > error code handling. Not sure why I didn't do that originally :( I think that would make sense as a followup. It'll probably have to change the callback's signature, though. --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
On Mon, Jan 18, 2016 at 10:09 AM, Andy Lutomirski <luto@amacapital.net> wrote: > On Mon, Jan 18, 2016 at 7:44 AM, Jean Delvare <jdelvare@suse.de> wrote: >>> + >>> + 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); >> >> The problem is not introduced by your patch, but the length check is >> inconsistent. sizeof(struct dell_bios_keymap_entry) is 4. > > Yes, but sizeof(struct dell_bios_keymap_table) is 6. No it's not. It's 4. And my math below was all messed up, too. You were exactly right, and I'll fix it. --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
Hi Andy, On Fri, 15 Jan 2016 12:00:02 -0800, Andy Lutomirski wrote: > UC means uncached. ioremap, on x86, asks for an uncached mapping, so > every memory access (load or store) hits main memory individually. > Assuming that the spec says that whatever physical memory the DMI > tables live in is permitted to be used with cached accesses, asking > for the CPU cache to be permitted on those accesses will make them a > whole lot faster. > > If that isn't safe, you could also just copy each table out of the > ioremap space into normal RAM as needed using MOVNTDQA. I forget what > the helper for that is called, but it basically does a fast streaming > IO read and then writes to normal RAM, memcpy style. Most modern CPUs > support it. I have no idea what is allowed and what isn't, sorry. You would have to check the SMBIOS specification but also the UEFI specification. I have to admit I never understood why dmi_alloc is arch-specific nor why dmi_remap is needed in the first place (and even less why dmi_early_remap is different.) So I'm not going to mess up with that code. I have no idea how costly dmi_remap() is, but certainly it is being called more and more as we can see dmi_walk() gaining in popularity. So if anyone is worried about the performance, I'll be happy to review and test patches.
On Mon, 18 Jan 2016 10:09:46 -0800, Andy Lutomirski wrote: > On Mon, Jan 18, 2016 at 7:44 AM, Jean Delvare <jdelvare@suse.de> wrote: > > On Sun, 3 Jan 2016 06:52:28 -0800, Andy Lutomirski wrote: > >> + if (results->err || results->keymap) > >> + return; /* We already found the hotkey table. */ > > > > Can this actually happen? > > Yes, I think, if Dell ships a laptop with two tables of type 0xB2. > There's no return code that says "I'm done", so I can't just stop > walking the DMI data after finding what I'm looking for. This may be another thing to consider when redesigning dmi_walk. Maybe we should let the callback function notify when processing should stop. If nothing else it should make things slightly faster by avoiding callbacks for the remaining entries. And it may allow for cleaner handling of corner cases. > (...) > I think the length check is correct, but the hotkey_num calculation is > wrong. The table is 84 bytes on my system, which makes perfect sense: > 6 bytes of header and 78 == 13*6 bytes of entries. But 84-4 is *not* > a multiple of 6. For the record, I don't have a Dell laptop myself but I have a DMI table dump from a Latitude E6410 and the type 178 record length is 96 bytes (4 + 23 * 4.) > > (...) > > I think it would make sense to fix dmi_walk() so that it lets the > > decoding function return error codes. This would avoid the > > convoluted error code handling. Not sure why I didn't do that > > originally :( > > I think that would make sense as a followup. It'll probably have to > change the callback's signature, though. Indeed. That won't be a nice and easy change, but it can still be done.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index 57402c4c394e..52db2721d7e3 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -116,7 +116,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 = { @@ -316,20 +319,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_table(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 < @@ -358,11 +375,13 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void) keymap[hotkey_num].type = KE_END; - return keymap; + results->err = 0; + 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(); @@ -373,20 +392,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_table, &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); @@ -413,15 +438,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; @@ -432,7 +448,6 @@ static int __init dell_wmi_init(void) return -ENODEV; } - dmi_walk(find_hk_type, NULL); acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor; err = dell_wmi_input_setup();
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. Cc: stable@vger.kernel.org Signed-off-by: Andy Lutomirski <luto@kernel.org> --- This seems to work on my laptop. It applies to platform-drivers-x86/for-next. drivers/platform/x86/dell-wmi.c | 69 +++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 27 deletions(-)