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

Groups > linux.kernel > #1311627 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

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

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