Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300318 > unrolled thread

[PATCH] dell-wmi: Stop storing pointers to DMI tables

Back to article view | Back to linux.kernel

Contents

  [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 Jean Delvare <jdelvare@suse.de> - 2016-01-15 14:30 +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 Jean Delvare <jdelvare@suse.de> - 2016-01-15 14:30 +0100
            Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-15 17:30 +0100
              Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-15 19:00 +0100
                Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-15 21:10 +0100
                  Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-19 10:20 +0100
    Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Michał Kępień <kernel@kempniu.pl> - 2016-01-14 11:30 +0100
    Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-18 16:50 +0100
      Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-18 19:20 +0100
      Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Andy Lutomirski <luto@amacapital.net> - 2016-01-18 19:20 +0100
        Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables Jean Delvare <jdelvare@suse.de> - 2016-01-19 10:20 +0100

#1300318 — [PATCH] dell-wmi: Stop storing pointers to DMI tables

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]

#1306734

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]

#1307479

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]

#1308872

[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]

#1309134

> > 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]

#1310108

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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1309299

> 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]

#1309624

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]

#1310104

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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1310251

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

[toc] | [prev] | [next] | [standalone]

#1310331

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?

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1310437

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

[toc] | [prev] | [next] | [standalone]

#1312034

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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1309159

> 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] | [next] | [standalone]

#1311627

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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1311732

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

[toc] | [prev] | [next] | [standalone]

#1311733

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

[toc] | [prev] | [next] | [standalone]

#1312032

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.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web