ATA: piix, fix pointer deref on suspend

PATCH Superseded Jiri Slaby 5 participants 17 messages Dec 3, 2008 – Dec 29, 2008

Subsystems (2)

libata Dmi/smbios support

lore

Share & tools

lore

Jiri Slaby

author

2008-12-03 08:15 UTC #0 lore raw

PATCH 1/1 ATA: piix, fix pointer deref on suspend

Hi,

I've found this issue in the mmotm 2008-12-02-17-08.

Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.

Check if they are null and avoid checks in that case.

Solves:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8043da97>] piix_pci_device_suspend+0x117/0x230

Signed-off-by Jiri Slaby <[email protected]>

Cc Jeff Garzik <[email protected]>

Cc Alexandru Romanescu <[email protected]>

Cc Tejun Heo <[email protected]>

---
 drivers/ata/ata_piix.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

drivers/ata/ata_piix.c +8 -1

index d6d97d8..c11936e 100644
@@ -1072,7 +1072,14 @@ static int piix_broken_suspend(void)
 	 * matching is necessary because dmi_system_id.matches is
 	 * limited to four entries.
 	 */
-	if (!strcmp(dmi_get_system_info(DMI_SYS_VENDOR), "TOSHIBA") &&
+	if (dmi_get_system_info(DMI_SYS_VENDOR) &&
+	    dmi_get_system_info(DMI_PRODUCT_NAME) &&
+	    dmi_get_system_info(DMI_PRODUCT_VERSION) &&
+	    dmi_get_system_info(DMI_PRODUCT_SERIAL) &&
+	    dmi_get_system_info(DMI_BOARD_VENDOR) &&
+	    dmi_get_system_info(DMI_BOARD_NAME) &&
+	    dmi_get_system_info(DMI_BOARD_VERSION) &&
+	    !strcmp(dmi_get_system_info(DMI_SYS_VENDOR), "TOSHIBA") &&
 	    !strcmp(dmi_get_system_info(DMI_PRODUCT_NAME), "000000") &&
 	    !strcmp(dmi_get_system_info(DMI_PRODUCT_VERSION), "000000") &&
 	    !strcmp(dmi_get_system_info(DMI_PRODUCT_SERIAL), "000000") &&

Signature · 1 line

1.6.0.4

Tejun Heo

Acked-by on v1 1/1

2008-12-03 09:04 UTC #1 lore raw

Quoted 13 lines · Jiri Slaby

Hi,
I've found this issue in the mmotm 2008-12-02-17-08.
--
Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.
Check if they are null and avoid checks in that case.

Quoted 8 lines

Solves:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8043da97>] piix_pci_device_suspend+0x117/0x230
Signed-off-by Jiri Slaby <[email protected]>
Cc Jeff Garzik <[email protected]>
Cc Alexandru Romanescu <[email protected]>
Cc Tejun Heo <[email protected]>

Thanks.

Acked-by Tejun Heo <[email protected]>

Signature · 1 line

tejun

Alan Cox

2008-12-03 10:14 UTC #2 lore raw

In reply to Jiri Slaby

Quoted 4 lines · On Wed, 3 Dec 2008 09:13:46 +0100 Jiri Slaby <[email protected]> wrote:

Hi,
I've found this issue in the mmotm 2008-12-02-17-08.

Quoted 8 lines

--
Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.
Check if they are null and avoid checks in that case.

Gak.. Add some kind of helper to the dmi code to do this and do the comparison.

Tejun Heo

2008-12-03 10:40 UTC #3 lore raw

Quoted 2 lines · Alan Cox

Gak.. Add some kind of helper to the dmi code to do this and do the comparison.

It would be best to increase or remove the match limit. Maybe we should just bump the limit to eight?

Signature · 1 line

tejun

Jeff Garzik

2008-12-09 05:48 UTC #4 lore raw

In reply to Jiri Slaby

Quoted 17 lines · Jiri Slaby

Hi,
I've found this issue in the mmotm 2008-12-02-17-08.
--
Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.
Check if they are null and avoid checks in that case.
Solves:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8043da97>] piix_pci_device_suspend+0x117/0x230

Quoted 7 lines

Signed-off-by Jiri Slaby <[email protected]>
Cc Jeff Garzik <[email protected]>
Cc Alexandru Romanescu <[email protected]>
Cc Tejun Heo <[email protected]>
---
 drivers/ata/ata_piix.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

applied #upstream-fixes (until someone creates a better patch...)

Jiri Slaby

author

2008-12-09 20:53 UTC #5 lore raw

PATCH 1/2 DMI: add dmi_match

Add a wrapper for testing system_info which will handle also NULL system infos.

This will be used by the ata PIIX driver.

Signed-off-by Jiri Slaby <[email protected]>

Cc Jeff Garzik <[email protected]>

Cc Alexandru Romanescu <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Alan Cox <[email protected]>

---
 include/linux/dmi.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

include/linux/dmi.h +17 -0

index 2bfda17..9bdb4b9 100644
@@ -48,6 +48,21 @@ extern int dmi_name_in_serial(const char *str);
 extern int dmi_available;
 extern int dmi_walk(void (*decode)(const struct dmi_header *));
 
+/**
+ * dmi_match - compare string to the dmi field (if exists)
+ *
+ * Returns true if requested field equals to str (including NULL).
+ */
+static inline bool dmi_match(enum dmi_field f, const char *str)
+{
+	const char *info = dmi_get_system_info(f);
+
+	if (info == NULL || str == NULL)
+		return info == str;
+
+	return !strcmp(info, str);
+}
+
 #else
 
 static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }

@@ -61,6 +76,8 @@ static inline int dmi_name_in_serial(const char *s) { return 0; }
 #define dmi_available 0
 static inline int dmi_walk(void (*decode)(const struct dmi_header *))
 	{ return -1; }
+static inline bool dmi_match(enum dmi_field f, const char *str)
+	{ return false; }
 
 #endif

Signature · 1 line

1.6.0.5

Jiri Slaby

author

2008-12-09 20:54 UTC #6 lore raw

PATCH v2 2/2 ATA: piix, fix pointer deref on suspend

Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.

Check if they are null and avoid checks in that case.

Solves:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8043da97>] piix_pci_device_suspend+0x117/0x230

Signed-off-by Jiri Slaby <[email protected]>

Cc Jeff Garzik <[email protected]>

Cc Alexandru Romanescu <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Alan Cox <[email protected]>

---
 drivers/ata/ata_piix.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

drivers/ata/ata_piix.c +7 -7

index d6d97d8..5fdf167 100644
@@ -1072,13 +1072,13 @@ static int piix_broken_suspend(void)
 	 * matching is necessary because dmi_system_id.matches is
 	 * limited to four entries.
 	 */
-	if (!strcmp(dmi_get_system_info(DMI_SYS_VENDOR), "TOSHIBA") &&
-	    !strcmp(dmi_get_system_info(DMI_PRODUCT_NAME), "000000") &&
-	    !strcmp(dmi_get_system_info(DMI_PRODUCT_VERSION), "000000") &&
-	    !strcmp(dmi_get_system_info(DMI_PRODUCT_SERIAL), "000000") &&
-	    !strcmp(dmi_get_system_info(DMI_BOARD_VENDOR), "TOSHIBA") &&
-	    !strcmp(dmi_get_system_info(DMI_BOARD_NAME), "Portable PC") &&
-	    !strcmp(dmi_get_system_info(DMI_BOARD_VERSION), "Version A0"))
+	if (dmi_match(DMI_SYS_VENDOR, "TOSHIBA") &&
+	    dmi_match(DMI_PRODUCT_NAME, "000000") &&
+	    dmi_match(DMI_PRODUCT_VERSION, "000000") &&
+	    dmi_match(DMI_PRODUCT_SERIAL, "000000") &&
+	    dmi_match(DMI_BOARD_VENDOR, "TOSHIBA") &&
+	    dmi_match(DMI_BOARD_NAME, "Portable PC") &&
+	    dmi_match(DMI_BOARD_VERSION, "Version A0"))
 		return 1;
 
 	return 0;

Signature · 1 line

1.6.0.5

Jeff Garzik

2008-12-09 21:29 UTC #7 lore raw

Quoted 11 lines · Jiri Slaby

Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.
Check if they are null and avoid checks in that case.
Solves:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8043da97>] piix_pci_device_suspend+0x117/0x230

Quoted 8 lines

Signed-off-by Jiri Slaby <[email protected]>
Cc Jeff Garzik <[email protected]>
Cc Alexandru Romanescu <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Alan Cox <[email protected]>
---
 drivers/ata/ata_piix.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

Note that the original version of this patch went upstream last night...

Jiri Slaby

author

2008-12-09 22:06 UTC #8 lore raw

Quoted 1 line · Jeff Garzik

Note that the original version of this patch went upstream last night...

Ah, I'll repost it based on that fact then. Also I'll deinline dmi_match as pointed out by Alan.

Jiri Slaby

author

2008-12-10 13:09 UTC #9 lore raw

PATCH 1/2 DMI: add dmi_match

In reply to Jeff Garzik

Add a wrapper for testing system_info which will handle also NULL system infos.

This will be used by the ata PIIX driver.

Signed-off-by Jiri Slaby <[email protected]>

Cc Jeff Garzik <[email protected]>

Cc Alexandru Romanescu <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Alan Cox <[email protected]>

---
 drivers/firmware/dmi_scan.c |   16 ++++++++++++++++
 include/linux/dmi.h         |    3 +++
 2 files changed, 19 insertions(+), 0 deletions(-)

drivers/firmware/dmi_scan.c +16 -0

index 4a597d8..78b989d 100644
@@ -582,3 +582,19 @@ int dmi_walk(void (*decode)(const struct dmi_header *))
 	return 0;
 }
 EXPORT_SYMBOL_GPL(dmi_walk);
+
+/**
+ * dmi_match - compare a string to the dmi field (if exists)
+ *
+ * Returns true if the requested field equals to the str (including NULL).
+ */
+bool dmi_match(enum dmi_field f, const char *str)
+{
+	const char *info = dmi_get_system_info(f);
+
+	if (info == NULL || str == NULL)
+		return info == str;
+
+	return !strcmp(info, str);
+}
+EXPORT_SYMBOL_GPL(dmi_match);

include/linux/dmi.h +3 -0

index 2bfda17..3416190 100644
@@ -47,6 +47,7 @@ extern int dmi_name_in_vendors(const char *str);
 extern int dmi_name_in_serial(const char *str);
 extern int dmi_available;
 extern int dmi_walk(void (*decode)(const struct dmi_header *));
+extern bool dmi_match(enum dmi_field f, const char *str);
 
 #else

@@ -61,6 +62,8 @@ static inline int dmi_name_in_serial(const char *s) { return 0; }
 #define dmi_available 0
 static inline int dmi_walk(void (*decode)(const struct dmi_header *))
 	{ return -1; }
+static inline bool dmi_match(enum dmi_field f, const char *str)
+	{ return false; }
 
 #endif

Signature · 1 line

1.6.0.5

Jiri Slaby

author

2008-12-10 13:10 UTC #10 lore raw

PATCH 2/2 ATA: piix, cleanup dmi strings checking

Commit
ATA: piix, fix pointer deref on suspend
fixed a possible oops in an ugly manner. Use newly introduced dmi_match() to make the code pretty again.

Signed-off-by Jiri Slaby <[email protected]>

Cc Jeff Garzik <[email protected]>

Cc Alexandru Romanescu <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Alan Cox <[email protected]>

---
 drivers/ata/ata_piix.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

drivers/ata/ata_piix.c +7 ~~-14~~

index c11936e..5fdf167 100644
@@ -1072,20 +1072,13 @@ static int piix_broken_suspend(void)
 	 * matching is necessary because dmi_system_id.matches is
 	 * limited to four entries.
 	 */
-	if (dmi_get_system_info(DMI_SYS_VENDOR) &&
-	    dmi_get_system_info(DMI_PRODUCT_NAME) &&
-	    dmi_get_system_info(DMI_PRODUCT_VERSION) &&
-	    dmi_get_system_info(DMI_PRODUCT_SERIAL) &&
-	    dmi_get_system_info(DMI_BOARD_VENDOR) &&
-	    dmi_get_system_info(DMI_BOARD_NAME) &&
-	    dmi_get_system_info(DMI_BOARD_VERSION) &&
-	    !strcmp(dmi_get_system_info(DMI_SYS_VENDOR), "TOSHIBA") &&
-	    !strcmp(dmi_get_system_info(DMI_PRODUCT_NAME), "000000") &&
-	    !strcmp(dmi_get_system_info(DMI_PRODUCT_VERSION), "000000") &&
-	    !strcmp(dmi_get_system_info(DMI_PRODUCT_SERIAL), "000000") &&
-	    !strcmp(dmi_get_system_info(DMI_BOARD_VENDOR), "TOSHIBA") &&
-	    !strcmp(dmi_get_system_info(DMI_BOARD_NAME), "Portable PC") &&
-	    !strcmp(dmi_get_system_info(DMI_BOARD_VERSION), "Version A0"))
+	if (dmi_match(DMI_SYS_VENDOR, "TOSHIBA") &&
+	    dmi_match(DMI_PRODUCT_NAME, "000000") &&
+	    dmi_match(DMI_PRODUCT_VERSION, "000000") &&
+	    dmi_match(DMI_PRODUCT_SERIAL, "000000") &&
+	    dmi_match(DMI_BOARD_VENDOR, "TOSHIBA") &&
+	    dmi_match(DMI_BOARD_NAME, "Portable PC") &&
+	    dmi_match(DMI_BOARD_VERSION, "Version A0"))
 		return 1;
 
 	return 0;

Signature · 1 line

1.6.0.5

Jeff Garzik

2008-12-29 12:41 UTC #11 lore raw

Re: [PATCH 1/2] DMI: add dmi_match

In reply to Jiri Slaby

Quoted 10 lines · Jiri Slaby

Add a wrapper for testing system_info which will handle also NULL system infos.
This will be used by the ata PIIX driver.
Signed-off-by Jiri Slaby <[email protected]>
Cc Jeff Garzik <[email protected]>
Cc Alexandru Romanescu <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Alan Cox <[email protected]>

Quoted 4 lines

---
 drivers/firmware/dmi_scan.c |   16 ++++++++++++++++
 include/linux/dmi.h         |    3 +++
 2 files changed, 19 insertions(+), 0 deletions(-)

applied 1-2

Andrew Morton

2008-12-11 20:28 UTC #12 lore raw

In reply to Jiri Slaby

Quoted 10 lines · On Tue, 9 Dec 2008 21:52:28 +0100 Jiri Slaby <[email protected]> wrote:

Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.
Check if they are null and avoid checks in that case.
Solves:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8043da97>] piix_pci_device_suspend+0x117/0x230

This already got fixed, the nasty way.

I queued "DMI: add dmi_match". We can later switch the piix nastyfix over to the nicefix as a cleanup.

Jiri Slaby

author

2008-12-11 20:40 UTC #13 lore raw

Andrew Morton napsal(a):

Quoted 10 lines

Quoted 7 lines · On Tue, 9 Dec 2008 21:52:28 +0100 Jiri Slaby <[email protected]> wrote:
Commit
ata_piix: add borked Tecra M4 to broken suspend list introduced DMI variables checking, but they can be null, so that we possibly dereference null.
Check if they are null and avoid checks in that case.

Quoted 8 lines

Quoted 3 lines
Solves:
BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
IP: [<ffffffff8043da97>] piix_pci_device_suspend+0x117/0x230
This already got fixed, the nasty way.
I queued "DMI: add dmi_match". We can later switch the piix nastyfix over to the nicefix as a cleanup.

Oooops, I didn't Cced you to the last version, sorry for that.

Alan Cox

2008-12-09 21:00 UTC #14 lore raw

Re: [PATCH 1/2] DMI: add dmi_match

In reply to Jiri Slaby

include/linux/dmi.h extern int dmi_name_in_serial(const char *str); · line 63

Quoted 9 lines

include/linux/dmi.h +8 -0

+static inline bool dmi_match(enum dmi_field f, const char *str)
+{
+	const char *info = dmi_get_system_info(f);
+
+	if (info == NULL || str == NULL)
+		return info == str;
+
+	return !strcmp(info, str);

Not sure it is worth inlining but its definitely better this way

ACK

Tejun Heo

Acked-by on v1 1/2

2008-12-10 02:01 UTC #15 lore raw

Re: [PATCH 1/2] DMI: add dmi_match

In reply to Jiri Slaby

Quoted 10 lines · Jiri Slaby

Add a wrapper for testing system_info which will handle also NULL system infos.
This will be used by the ata PIIX driver.
Signed-off-by Jiri Slaby <[email protected]>
Cc Jeff Garzik <[email protected]>
Cc Alexandru Romanescu <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Alan Cox <[email protected]>

Yeah, about ten times prettier this way.

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Andrew Morton

2008-12-11 20:30 UTC #16 lore raw

Re: [PATCH 1/2] DMI: add dmi_match

In reply to Jiri Slaby

Quoted 6 lines · On Tue, 9 Dec 2008 21:52:27 +0100 Jiri Slaby <[email protected]> wrote:

+/**
+ * dmi_match - compare string to the dmi field (if exists)
+ *
+ * Returns true if requested field equals to str (including NULL).
+ */

Forgot to document the arguments. If you think it matters. (I don't).

include/linux/dmi.h extern int dmi_name_in_serial(const char *str); · line 65

Quoted 10 lines

include/linux/dmi.h +10 -0

+static inline bool dmi_match(enum dmi_field f, const char *str)
+{
+	const char *info = dmi_get_system_info(f);
+
+	if (info == NULL || str == NULL)
+		return info == str;
+
+	return !strcmp(info, str);
+}
+

I suspect we'd have thinner kernel if this wasn't inlined.