Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1300318 > unrolled thread
| Started by | Andy Lutomirski <luto@kernel.org> |
|---|---|
| First post | 2016-01-03 16:00 +0100 |
| Last post | 2016-01-14 11:30 +0100 |
| Articles | 8 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@kernel.org> - 2016-01-03 16:00 +0100
Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-11 23:00 +0100
Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Pali Rohár <pali.rohar@gmail.com> - 2016-01-12 15:30 +0100
Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-13 23:30 +0100
Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Michał Kępień <kernel@kempniu.pl> - 2016-01-14 11:00 +0100
Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-01-14 15:10 +0100
Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-14 21:20 +0100
Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Michał Kępień <kernel@kempniu.pl> - 2016-01-14 11:30 +0100
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2016-01-03 16:00 +0100 |
| Subject | [PATCH] dell-wmi: Stop storing pointers to DMI tables |
| Message-ID | <qMSiu-6Jn-11@gated-at.bofh.it> |
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;
+};
/* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-01-11 23:00 +0100 |
| Message-ID | <qPSFk-8fi-7@gated-at.bofh.it> |
| In reply to | #1300318 |
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
>
--
Andy Lutomirski
AMA Capital Management, LLC
[toc] | [prev] | [next] | [standalone]
| From | Pali Rohár <pali.rohar@gmail.com> |
|---|---|
| Date | 2016-01-12 15:30 +0100 |
| Message-ID | <qQ87p-28L-35@gated-at.bofh.it> |
| In reply to | #1306734 |
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
> >
>
>
>
--
Pali Rohár
pali.rohar@gmail.com
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@amacapital.net> |
|---|---|
| Date | 2016-01-13 23:30 +0100 |
| Message-ID | <qQC5s-67v-5@gated-at.bofh.it> |
| In reply to | #1307479 |
[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
--
Andy Lutomirski
AMA Capital Management, LLC
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2016-01-14 11:00 +0100 |
| Message-ID | <qQMRc-5dh-23@gated-at.bofh.it> |
| In reply to | #1308872 |
> > 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... -- Best regards, Michał Kępień
[toc] | [prev] | [next] | [standalone]
| From | One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> |
|---|---|
| Date | 2016-01-14 15:10 +0100 |
| Message-ID | <qQQL8-8bA-11@gated-at.bofh.it> |
| In reply to | #1308872 |
> 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
[toc] | [prev] | [next] | [standalone]
| From | Jean Delvare <jdelvare@suse.de> |
|---|---|
| Date | 2016-01-14 21:20 +0100 |
| Message-ID | <qQWxb-3FW-1@gated-at.bofh.it> |
| In reply to | #1309299 |
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. -- Jean Delvare SUSE L3 Support
[toc] | [prev] | [next] | [standalone]
| From | Michał Kępień <kernel@kempniu.pl> |
|---|---|
| Date | 2016-01-14 11:30 +0100 |
| Message-ID | <qQNke-5Dw-23@gated-at.bofh.it> |
| In reply to | #1300318 |
> 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 *.
--
Best regards,
Michał Kępień
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web