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

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

[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

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
Cc: Tejun Heo <tj@kernel.org>
---
 drivers/ata/ata_piix.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index d6d97d8..c11936e 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -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") &&
-- 
1.6.0.4

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

[Tejun Heo]

Jiri Slaby wrote:
> 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 <jirislaby@gmail.com>
> Cc: Jeff Garzik <jgarzik@redhat.com>
> Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
> Cc: Tejun Heo <tj@kernel.org>

Thanks.

Acked-by: Tejun Heo <tj@kernel.org>

-- 
tejun

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

[Alan Cox]

On Wed,  3 Dec 2008 09:13:46 +0100
Jiri Slaby <jirislaby@gmail.com> wrote:

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

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

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

[Tejun Heo]

Alan Cox wrote:
> 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?

-- 
tejun

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

[Jeff Garzik]

Jiri Slaby wrote:
> 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 <jirislaby@gmail.com>
> Cc: Jeff Garzik <jgarzik@redhat.com>
> Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
> Cc: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ata/ata_piix.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)

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

[PATCH 1/2] DMI: add dmi_match

[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 <jirislaby@gmail.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
Cc: Tejun Heo <tj@kernel.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 include/linux/dmi.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 2bfda17..9bdb4b9 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -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
 
-- 
1.6.0.5

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

[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

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
Cc: Tejun Heo <tj@kernel.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/ata_piix.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index d6d97d8..5fdf167 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -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;
-- 
1.6.0.5

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

[Jeff Garzik]

Jiri Slaby 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
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> Cc: Jeff Garzik <jgarzik@redhat.com>
> Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
>  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...

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

[Jiri Slaby]

Jeff Garzik wrote:
> 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.

[PATCH 1/2] DMI: add dmi_match

[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 <jirislaby@gmail.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
Cc: Tejun Heo <tj@kernel.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/firmware/dmi_scan.c |   16 ++++++++++++++++
 include/linux/dmi.h         |    3 +++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 4a597d8..78b989d 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -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);
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 2bfda17..3416190 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -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
 
-- 
1.6.0.5

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

[Jiri Slaby]

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 <jirislaby@gmail.com>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
Cc: Tejun Heo <tj@kernel.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/ata/ata_piix.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index c11936e..5fdf167 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -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;
-- 
1.6.0.5

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

[Jeff Garzik]

Jiri Slaby wrote:
> 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 <jirislaby@gmail.com>
> Cc: Jeff Garzik <jgarzik@redhat.com>
> Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
>  drivers/firmware/dmi_scan.c |   16 ++++++++++++++++
>  include/linux/dmi.h         |    3 +++
>  2 files changed, 19 insertions(+), 0 deletions(-)

applied 1-2

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

[Andrew Morton]

On Tue,  9 Dec 2008 21:52:28 +0100
Jiri Slaby <jirislaby@gmail.com> 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.

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

[Jiri Slaby]

Andrew Morton napsal(a):
> On Tue,  9 Dec 2008 21:52:28 +0100
> Jiri Slaby <jirislaby@gmail.com> 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.

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

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

[Alan Cox]

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

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

[Tejun Heo]

Jiri Slaby wrote:
> 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 <jirislaby@gmail.com>
> Cc: Jeff Garzik <jgarzik@redhat.com>
> Cc: Alexandru Romanescu <a_romanescu@yahoo.co.uk>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

Yeah, about ten times prettier this way.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Andrew Morton]

On Tue,  9 Dec 2008 21:52:27 +0100
Jiri Slaby <jirislaby@gmail.com> 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).

> +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.