dell-wmi: DMI misuse fixes

PATCH Superseded Andy Lutomirski 4 participants 15 messages Jan 18, 2016 – Jan 19, 2016

Subsystems (2)

x86 platform drivers Dmi/smbios support

lore

Share & tools

lore

Andy Lutomirski

author

2016-01-18 20:59 UTC #8 lore raw

PATCH v2 3/3 dmi: Make dmi_walk and dmi_walk_early return real error codes

In reply to Andy Lutomirski

Currently they return -1 on error, which will confuse callers if they try to interpret it as a normal negative error code.

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/firmware/dmi_scan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/firmware/dmi_scan.c +3 -3

index 0e08e665f715..451ac04ed18d 100644
@@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 
 	buf = dmi_early_remap(dmi_base, orig_dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, NULL);

@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;
 
 	buf = dmi_remap(dmi_base, dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, private_data);

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-01-18 20:59 UTC #0 lore raw

v2 cover dell-wmi: DMI misuse fixes

Darren, patches 1 and 2 are for platform-drivers-x86, assuming that people like them when reviewed. They're probably 4.5 material, since they fix a real bug, even if it's hard to trigger.

Jean, patch 3 is for you if you like it.

Changes from v1:
- Patch 1 has minor tweaks from Jean.
- Patches 2 and 3 are new.

Andy Lutomirski (3):
dell-wmi: Stop storing pointers to DMI tables
dell-wmi: Fix hotkey table size check
dmi: Make dmi_walk and dmi_walk_early return real error codes

 drivers/firmware/dmi_scan.c     |  6 ++--
 drivers/platform/x86/dell-wmi.c | 71 +++++++++++++++++++++++++----------------
 2 files changed, 46 insertions(+), 31 deletions(-)

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-01-18 20:59 UTC #1 lore raw

PATCH v2 1/3 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.

Signed-off-by Andy Lutomirski <[email protected]>

---

Notes:
Changes from v1:
- Rename handle_dmi_table to handle_dmi_entry (Jean)
- Remove useless assignment to results->err (Jean)

 drivers/platform/x86/dell-wmi.c | 68 +++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 27 deletions(-)

drivers/platform/x86/dell-wmi.c +41 ~~-27~~

index b6f193b07566..5c0d037fcd40 100644
@@ -114,7 +114,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 = {

@@ -315,20 +318,34 @@ static void dell_wmi_notify(u32 value, void *context)
 	kfree(obj);
 }
 
-static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
+static void __init handle_dmi_entry(const struct dmi_header *dm,
+				    void *opaque)
 {
-	int hotkey_num = (dell_bios_hotkey_table->header.length - 4) /
-				sizeof(struct dell_bios_keymap_entry);
+	struct dell_dmi_results *results = opaque;
+	struct dell_bios_hotkey_table *table;
 	struct key_entry *keymap;
-	int i;
+	int hotkey_num, i;
+
+	if (results->err || results->keymap)
+		return;		/* We already found the hotkey table. */
+
+	if (dm->type != 0xb2 || dm->length <= 6)
+		return;
+
+	table = container_of(dm, struct dell_bios_hotkey_table, header);
+
+	hotkey_num = (table->header.length - 4) /
+				sizeof(struct dell_bios_keymap_entry);
 
 	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);
-	if (!keymap)
-		return NULL;
+	if (!keymap) {
+		results->err = -ENOMEM;
+		return;
+	}
 
 	for (i = 0; i < hotkey_num; i++) {
 		const struct dell_bios_keymap_entry *bios_entry =
-					&dell_bios_hotkey_table->keymap[i];
+					&table->keymap[i];
 
 		/* Uninitialized entries are 0 aka KEY_RESERVED. */
 		u16 keycode = (bios_entry->keycode <

@@ -357,11 +374,12 @@ static const struct key_entry * __init dell_wmi_prepare_new_keymap(void)
 
 	keymap[hotkey_num].type = KE_END;
 
-	return keymap;
+	results->keymap = keymap;
 }
 
 static int __init dell_wmi_input_setup(void)
 {
+	struct dell_dmi_results dmi_results = {};
 	int err;
 
 	dell_wmi_input_dev = input_allocate_device();

@@ -372,20 +390,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_entry, &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);

@@ -412,15 +436,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;

@@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
-	dmi_walk(find_hk_type, NULL);
 
 	err = dell_wmi_input_setup();
 	if (err)

Signature · 1 line

2.5.0

Andy Lutomirski

author

2016-01-18 20:59 UTC #5 lore raw

PATCH v2 2/3 dell-wmi: Fix hotkey table size check

In reply to Andy Lutomirski

The minimum size of the table is 4, not 6. Replace the hard-coded number with a sizeof expression. While we're at it, repace the hard-coded 4 below as well.

Reported-by Jean Delvare <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 drivers/platform/x86/dell-wmi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/platform/x86/dell-wmi.c +4 -3

index 5c0d037fcd40..48838942d593 100644
@@ -111,7 +111,6 @@ struct dell_bios_keymap_entry {
 struct dell_bios_hotkey_table {
 	struct dmi_header header;
 	struct dell_bios_keymap_entry keymap[];
-
 };
 
 struct dell_dmi_results {

@@ -329,12 +328,14 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
 
-	if (dm->type != 0xb2 || dm->length <= 6)
+	if (dm->type != 0xb2 ||
+	    dm->length <= sizeof(struct dell_bios_hotkey_table))
 		return;
 
 	table = container_of(dm, struct dell_bios_hotkey_table, header);
 
-	hotkey_num = (table->header.length - 4) /
+	hotkey_num = (table->header.length -
+		      sizeof(struct dell_bios_hotkey_table)) /
 				sizeof(struct dell_bios_keymap_entry);
 
 	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);

Signature · 1 line

2.5.0

Jean Delvare

2016-01-19 07:54 UTC #9 lore raw

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

In reply to Andy Lutomirski

Hi Andy,

Quoted 22 lines · Andy Lutomirski

Currently they return -1 on error, which will confuse callers if they try to interpret it as a normal negative error code.
Signed-off-by Andy Lutomirski <[email protected]>
---
 drivers/firmware/dmi_scan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/firmware/dmi_scan.c +1 -1
index 0e08e665f715..451ac04ed18d 100644
@@ -144,7 +144,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 
 	buf = dmi_early_remap(dmi_base, orig_dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, NULL);
 

drivers/firmware/dmi_scan.c int dmi_walk(void (*decode)(const struct dmi_header *, void *), · line 981

Quoted 5 lines

drivers/firmware/dmi_scan.c +1 -1

@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;

-ENOSYS would seem more appropriate?

drivers/firmware/dmi_scan.c int dmi_walk(void (*decode)(const struct dmi_header *, void *), · line 988

Quoted 8 lines

drivers/firmware/dmi_scan.c +1 -1

 	buf = dmi_remap(dmi_base, dmi_len);
 	if (buf == NULL)
-		return -1;
+		return -ENOMEM;
 
 	dmi_decode_table(buf, decode, private_data);

There is a formatted comment before dmi_walk, which says "Returns -1 when the DMI table can't be reached". This comment needs to be updated.

Please also update the dmi_walk stub in include/linux/dmi.h when CONFIG_DMI isn't set.

I don't think this patch should be in this series, I'd rather take it in my dmi tree and push it to Linus immediately.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jean Delvare

2016-01-19 08:31 UTC #6 lore raw

Re: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check

In reply to Andy Lutomirski

Hi Andy,

Quoted 15 lines · Andy Lutomirski

The minimum size of the table is 4, not 6. Replace the hard-coded number with a sizeof expression. While we're at it, repace the hard-coded 4 below as well.
Reported-by Jean Delvare <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 drivers/platform/x86/dell-wmi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/platform/x86/dell-wmi.c +0 -0
index 5c0d037fcd40..48838942d593 100644

drivers/platform/x86/dell-wmi.c struct dell_bios_keymap_entry { · line 116

Quoted 7 lines

drivers/platform/x86/dell-wmi.c +0 -1

@@ -111,7 +111,6 @@ struct dell_bios_keymap_entry {
 struct dell_bios_hotkey_table {
 	struct dmi_header header;
 	struct dell_bios_keymap_entry keymap[];
-
 };
 
 struct dell_dmi_results {

Nice cleanup but in general we recommend to not mix style cleanups with functional changes. If you want to clean up dell-wmi you could do it in a separate patch and maybe include the fixes suggested by checkpatch.pl -f.

drivers/platform/x86/dell-wmi.c static void __init handle_dmi_entry(const struct dmi_header *dm, · line 333

Quoted 8 lines

drivers/platform/x86/dell-wmi.c +2 -1

@@ -329,12 +328,14 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
 	if (results->err || results->keymap)
 		return;		/* We already found the hotkey table. */
 
-	if (dm->type != 0xb2 || dm->length <= 6)
+	if (dm->type != 0xb2 ||
+	    dm->length <= sizeof(struct dell_bios_hotkey_table))
 		return;

I'm confused. sizeof(struct dell_bios_hotkey_table) is 4. Given that dm->length is guaranteed to be at least 4 per the SMBIOS specification, you are really only testing that dm->length != 4. Which means you are still accepting 5, 6 and 7, even though they would lead to hotkey_num = 0 below.

If the purpose of this check is only to guarantee that the container_of below is valid then you should check for dm->length < sizeof(struct dell_bios_hotkey_table) (not <=.) This is still useless in practice but I can understand and accept it because it is conceptually correct.

OTOH if the purpose of the check is to ensure that there is at least one hotkey, you should check for dm->length < sizeof(struct dell_bios_hotkey_table) + sizeof(struct dell_bios_keymap_entry) instead. hotkey_num could also be checked separately below but it is more efficient to have a single test.

Quoted 9 lines

drivers/platform/x86/dell-wmi.c +2 -1

 	table = container_of(dm, struct dell_bios_hotkey_table, header);
 
-	hotkey_num = (table->header.length - 4) /
+	hotkey_num = (table->header.length -
+		      sizeof(struct dell_bios_hotkey_table)) /
 				sizeof(struct dell_bios_keymap_entry);
 
 	keymap = kcalloc(hotkey_num + 1, sizeof(struct key_entry), GFP_KERNEL);

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Pali Rohár

2016-01-19 08:36 UTC #10 lore raw

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

In reply to Jean Delvare

Quoted 8 lines · On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:

drivers/firmware/dmi_scan.c int dmi_walk(void (*decode)(const struct dmi_header *, void *), · line 981
Quoted 6 lines
drivers/firmware/dmi_scan.c +1 -1
@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;
-ENOSYS would seem more appropriate?

IIRC -ENOSYS is for non implemented syscalls.

Signature · 2 lines

Pali Rohár
[email protected]

Jean Delvare

Reviewed-by on v2 1/3

2016-01-19 08:41 UTC #2 lore raw

Re: [PATCH v2 1/3] dell-wmi: Stop storing pointers to DMI tables

In reply to Andy Lutomirski

Hi Andy,

Quoted 22 lines · Andy Lutomirski

The dmi_walk function maps the DMI table, walks it, and unmaps it. This means that the dell_bios_hotkey_table that find_hk_type stores points to unmapped memory by the time it gets read.
I've been able to trigger crashes caused by the stale pointer a couple of times, but never on a stock kernel.
Fix it by generating the keymap in the dmi_walk callback instead of storing a pointer.
Signed-off-by Andy Lutomirski <[email protected]>
---
Notes:
Changes from v1:
- Rename handle_dmi_table to handle_dmi_entry (Jean)
- Remove useless assignment to results->err (Jean)
 drivers/platform/x86/dell-wmi.c | 68 +++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 27 deletions(-)
(...)

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 449

Quoted 5 lines

drivers/platform/x86/dell-wmi.c +0 -1

@@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
-	dmi_walk(find_hk_type, NULL);

Please also delete the following blank line, to avoid two consecutive blank lines.

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 451

Quoted 2 lines

drivers/platform/x86/dell-wmi.c +0 -0
 	err = dell_wmi_input_setup();
 	if (err)

Other than this, it looks good to me, so after fixing the above, you can add:

Reviewed-by Jean Delvare <[email protected]>

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jean Delvare

2016-01-19 09:03 UTC #11 lore raw

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

In reply to Pali Rohár

Hi Pali,

Quoted 11 lines · Pali Rohár

Quoted 8 lines · On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
drivers/firmware/dmi_scan.c int dmi_walk(void (*decode)(const struct dmi_header *, void *), · line 981
Quoted 6 lines
drivers/firmware/dmi_scan.c +1 -1
@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;
-ENOSYS would seem more appropriate?
IIRC -ENOSYS is for non implemented syscalls.

I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when a specific subsystem is not included. Not related to syscalls at all. This is what lead to my suggestion.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Pali Rohár

2016-01-19 09:07 UTC #12 lore raw

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

Quoted 13 lines · On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote:

Hi Pali,
Quoted 10 lines · Pali Rohár
Quoted 8 lines · On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
drivers/firmware/dmi_scan.c int dmi_walk(void (*decode)(const struct dmi_header *, void *), · line 981
Quoted 6 lines
drivers/firmware/dmi_scan.c +1 -1
@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;
-ENOSYS would seem more appropriate?

Quoted 5 lines

Quoted 1 line
IIRC -ENOSYS is for non implemented syscalls.
I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when a specific subsystem is not included. Not related to syscalls at all. This is what lead to my suggestion.

https://lkml.org/lkml/2014/8/22/492

Signature · 2 lines

Pali Rohár
[email protected]

Jean Delvare

2016-01-19 09:40 UTC #13 lore raw

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

Quoted 14 lines · Pali Rohár

Quoted 13 lines · On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote:
Hi Pali,
Quoted 10 lines · Pali Rohár
Quoted 8 lines · On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
drivers/firmware/dmi_scan.c int dmi_walk(void (*decode)(const struct dmi_header *, void *), · line 981
Quoted 6 lines
drivers/firmware/dmi_scan.c +1 -1
@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	u8 *buf;
 
 	if (!dmi_available)
-		return -1;
+		return -ENOENT;
-ENOSYS would seem more appropriate?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
IIRC -ENOSYS is for non implemented syscalls.
I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when a specific subsystem is not included. Not related to syscalls at all. This is what lead to my suggestion.
https://lkml.org/lkml/2014/8/22/492

Thanks for the pointer, I wasn't aware of that.

It really should be documented. No, checkpatch.pl isn't documentation.

Also the commit sadly doesn't say why using ENOSYS in other contexts is considered a bad thing. What actual trouble did it cause?

Are the current presumably incorrect uses of ENOSYS ultimately going to be fixed? If not, I see no point in preventing other use cases.

Also what about errno(3)? It says ENOSYS is "Function not implemented"
- no mention to syscalls. And glibc agrees (set errno to ENOSYS and call perror, it says "Function not implemented.") In contradiction with the proposed rule.

Back to the patch under review, I would argue that ENOENT is solely for file-related operations (after all the man page says "No such file or directory"), which is why I did not like it. Then what is left to mean "function not available"? ENOTSUP?

Whatever the answer is, it should also be documented and added to the checkpatch warning message (if we stick to that plan...) If you tell people "don't use this" without telling them what to use instead, each of us will come up with something different and this will lead to inconsistency. Which is worse than everybody using the same (maybe suboptimal) error code.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jean Delvare

2016-01-19 11:04 UTC #3 lore raw

Re: [PATCH v2 1/3] dell-wmi: Stop storing pointers to DMI tables

In reply to Andy Lutomirski

Hi Andy,

One more thing I just noticed...

Quoted 15 lines · Andy Lutomirski

drivers/platform/x86/dell-wmi.c +3 -6

@@ -372,20 +390,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_entry, &dmi_results);
+	if (err)
+		goto err_free_dev;
(...)

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 451

Quoted 7 lines

drivers/platform/x86/dell-wmi.c +0 -1

@@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
 		return -ENODEV;
 	}
 
-	dmi_walk(find_hk_type, NULL);
 
 	err = dell_wmi_input_setup();
 	if (err)

Before, in the absence of DMI support, the driver would load and assume the old hotkey interface. After your change, the driver will fail to load if DMI support is missing. This could, in theory, cause a regression (although I would be very surprised if even the oldest supported models don't implement DMI.)

If you want this patch to make it into stable, you probably should stick to the original behavior and move that change (if you still believe it is a good idea) to a separate patch, and at the same time make DELL_WMI either select or depend on DMI. Many other platform/x86 drivers should do the same, BTW, but that's a separate topic.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Andy Lutomirski

2016-01-19 18:29 UTC #4 lore raw

Re: [PATCH v2 1/3] dell-wmi: Stop storing pointers to DMI tables

Quoted 25 lines · Jean Delvare

Hi Andy,

One more thing I just noticed...

Quoted 20 lines · Andy Lutomirski

drivers/platform/x86/dell-wmi.c +3 -7

@@ -372,20 +390,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_entry, &dmi_results);
+     if (err)
+             goto err_free_dev;
(...)

@@ -431,7 +446,6 @@ static int __init dell_wmi_init(void)
              return -ENODEV;
      }
 
-     dmi_walk(find_hk_type, NULL);

Quoted 8 lines

drivers/platform/x86/dell-wmi.c static int __init dell_wmi_init(void) · line 451
Quoted 2 lines
drivers/platform/x86/dell-wmi.c +0 -0
      err = dell_wmi_input_setup();
      if (err)
Before, in the absence of DMI support, the driver would load and assume the old hotkey interface. After your change, the driver will fail to load if DMI support is missing. This could, in theory, cause a regression (although I would be very surprised if even the oldest supported models don't implement DMI.)

I switched it from a failure to a pr_warn.

Quoted 7 lines

If you want this patch to make it into stable, you probably should stick to the original behavior and move that change (if you still believe it is a good idea) to a separate patch, and at the same time make DELL_WMI either select or depend on DMI. Many other platform/x86 drivers should do the same, BTW, but that's a separate topic.

I'm going to add a patch for that to the series, without Cc: stable. I don't think it's a big enough deal to be worth -stable churn.

--Andy

Andy Lutomirski

2016-01-19 18:42 UTC #7 lore raw

Re: [PATCH v2 2/3] dell-wmi: Fix hotkey table size check

In reply to Jean Delvare

Quoted 24 lines · Jean Delvare

Hi Andy,
Quoted 21 lines · Andy Lutomirski
The minimum size of the table is 4, not 6. Replace the hard-coded number with a sizeof expression. While we're at it, repace the hard-coded 4 below as well.
Reported-by Jean Delvare <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 drivers/platform/x86/dell-wmi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/platform/x86/dell-wmi.c +0 -1
index 5c0d037fcd40..48838942d593 100644
@@ -111,7 +111,6 @@ struct dell_bios_keymap_entry {
 struct dell_bios_hotkey_table {
      struct dmi_header header;
      struct dell_bios_keymap_entry keymap[];
-
 };
 

Quoted 6 lines

drivers/platform/x86/dell-wmi.c struct dell_bios_keymap_entry { · line 116
Quoted 1 line
drivers/platform/x86/dell-wmi.c +0 -0
 struct dell_dmi_results {
Nice cleanup but in general we recommend to not mix style cleanups with functional changes. If you want to clean up dell-wmi you could do it in a separate patch and maybe include the fixes suggested by checkpatch.pl -f.

/me sheepishly puts the newline back in.

Quoted 21 lines

drivers/platform/x86/dell-wmi.c static void __init handle_dmi_entry(const struct dmi_header *dm, · line 333
Quoted 9 lines
drivers/platform/x86/dell-wmi.c +2 -1
@@ -329,12 +328,14 @@ static void __init handle_dmi_entry(const struct dmi_header *dm,
      if (results->err || results->keymap)
              return;         /* We already found the hotkey table. */
 
-     if (dm->type != 0xb2 || dm->length <= 6)
+     if (dm->type != 0xb2 ||
+         dm->length <= sizeof(struct dell_bios_hotkey_table))
              return;
I'm confused. sizeof(struct dell_bios_hotkey_table) is 4. Given that dm->length is guaranteed to be at least 4 per the SMBIOS specification, you are really only testing that dm->length != 4. Which means you are still accepting 5, 6 and 7, even though they would lead to hotkey_num = 0 below.
If the purpose of this check is only to guarantee that the container_of below is valid then you should check for dm->length < sizeof(struct dell_bios_hotkey_table) (not <=.) This is still useless in practice but I can understand and accept it because it is conceptually correct.

Quoted 5 lines

OTOH if the purpose of the check is to ensure that there is at least one hotkey, you should check for dm->length < sizeof(struct dell_bios_hotkey_table) + sizeof(struct dell_bios_keymap_entry) instead. hotkey_num could also be checked separately below but it is more efficient to have a single test.

I think the check is just to see if the buffer is big enough, but maybe there's history here, and I don't want to be the old to break ancient laptops for the sake of a cleanup. Let me try this again.

--Andy

Andy Lutomirski

2016-01-19 23:52 UTC #14 lore raw

Re: [PATCH v2 3/3] dmi: Make dmi_walk and dmi_walk_early return real error codes

In reply to Jean Delvare

Quoted 21 lines · Jean Delvare

Quoted 20 lines · Pali Rohár
Quoted 18 lines · On Tuesday 19 January 2016 10:03:03 Jean Delvare wrote:
Hi Pali,
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Tuesday 19 January 2016 08:54:26 Jean Delvare wrote:
drivers/firmware/dmi_scan.c int dmi_walk(void (*decode)(const struct dmi_header *, void *), · line 981
Quoted 6 lines
drivers/firmware/dmi_scan.c +1 -1
@@ -978,11 +978,11 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
      u8 *buf;
 
      if (!dmi_available)
-             return -1;
+             return -ENOENT;
-ENOSYS would seem more appropriate?
IIRC -ENOSYS is for non implemented syscalls.
I can see a lot of -ENOSYS in include/linux/*.h returned by stubs when a specific subsystem is not included. Not related to syscalls at all. This is what lead to my suggestion.

Quoted 8 lines

Quoted 1 line
https://lkml.org/lkml/2014/8/22/492
Thanks for the pointer, I wasn't aware of that.
It really should be documented. No, checkpatch.pl isn't documentation.
Also the commit sadly doesn't say why using ENOSYS in other contexts is considered a bad thing. What actual trouble did it cause?

The trouble is that user code likes to assume that, when a syscall returns -ENOSYS, that syscall isn't implemented. Letting ENOSYS leak out to userspace via a syscall that *is* implemented can confused things.

Quoted 3 lines

Are the current presumably incorrect uses of ENOSYS ultimately going to be fixed? If not, I see no point in preventing other use cases.

We at least want to prevent it from newly introduced syscalls.

I'll try to clean up the docs.

--Andy