All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
@ 2015-03-20  8:59 Jean Delvare
  2015-03-20  9:21 ` Ard Biesheuvel
  2015-03-26 13:06 ` Matt Fleming
  0 siblings, 2 replies; 8+ messages in thread
From: Jean Delvare @ 2015-03-20  8:59 UTC (permalink / raw)
  To: LKML; +Cc: Matt Fleming, Ard Biesheuvel, Ivan Khoronzhuk

dmi_num is a u16, dmi_len is a u32, so this construct:

	dmi_num = dmi_len / 4;

would result in an integer overflow for a DMI table larger than
256 kB. I've never see such a large table so far, but SMBIOS 3.0
makes it possible so maybe we'll see such tables in the future.

So instead of faking a structure count when the entry point does
not provide it, adjust the loop condition in dmi_table() to properly
deal with the case where dmi_num is not set.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/firmware/dmi_scan.c |   22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

--- linux-4.0-rc4.orig/drivers/firmware/dmi_scan.c	2015-03-19 14:19:56.384426825 +0100
+++ linux-4.0-rc4/drivers/firmware/dmi_scan.c	2015-03-20 09:23:37.689542485 +0100
@@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len,
 	int i = 0;
 
 	/*
-	 *	Stop when we see all the items the table claimed to have
-	 *	OR we run off the end of the table (also happens)
+	 * Stop when we have seen all the items the table claimed to have
+	 * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run
+	 * off the end of the table (should never happen but sometimes does
+	 * on bogus implementations.)
 	 */
-	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
+	while ((!num || i < num) &&
+	       (data - buf + sizeof(struct dmi_header)) <= len) {
 		const struct dmi_header *dm = (const struct dmi_header *)data;
 
 		/*
@@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(co
 	if (memcmp(buf, "_SM3_", 5) == 0 &&
 	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
 		dmi_ver = get_unaligned_be16(buf + 7);
+		dmi_num = 0;			/* No longer specified */
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
 
-		/*
-		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
-		 * containing the number of structures present in the table.
-		 * Instead, it defines the table size as a maximum size, and
-		 * relies on the end-of-table structure type (#127) to be used
-		 * to signal the end of the table.
-		 * So let's define dmi_num as an upper bound as well: each
-		 * structure has a 4 byte header, so dmi_len / 4 is an upper
-		 * bound for the number of structures in the table.
-		 */
-		dmi_num = dmi_len / 4;
-
 		if (dmi_walk_early(dmi_decode) == 0) {
 			pr_info("SMBIOS %d.%d present.\n",
 				dmi_ver >> 8, dmi_ver & 0xFF);


-- 
Jean Delvare
SUSE L3 Support

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
  2015-03-20  8:59 [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow Jean Delvare
@ 2015-03-20  9:21 ` Ard Biesheuvel
  2015-03-26 13:06 ` Matt Fleming
  1 sibling, 0 replies; 8+ messages in thread
From: Ard Biesheuvel @ 2015-03-20  9:21 UTC (permalink / raw)
  To: Jean Delvare; +Cc: LKML, Matt Fleming, Ivan Khoronzhuk

On 20 March 2015 at 09:59, Jean Delvare <jdelvare@suse.de> wrote:
> dmi_num is a u16, dmi_len is a u32, so this construct:
>
>         dmi_num = dmi_len / 4;
>
> would result in an integer overflow for a DMI table larger than
> 256 kB. I've never see such a large table so far, but SMBIOS 3.0
> makes it possible so maybe we'll see such tables in the future.
>
> So instead of faking a structure count when the entry point does
> not provide it, adjust the loop condition in dmi_table() to properly
> deal with the case where dmi_num is not set.
>
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>


Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


> ---
>  drivers/firmware/dmi_scan.c |   22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
>
> --- linux-4.0-rc4.orig/drivers/firmware/dmi_scan.c      2015-03-19 14:19:56.384426825 +0100
> +++ linux-4.0-rc4/drivers/firmware/dmi_scan.c   2015-03-20 09:23:37.689542485 +0100
> @@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len,
>         int i = 0;
>
>         /*
> -        *      Stop when we see all the items the table claimed to have
> -        *      OR we run off the end of the table (also happens)
> +        * Stop when we have seen all the items the table claimed to have
> +        * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run
> +        * off the end of the table (should never happen but sometimes does
> +        * on bogus implementations.)
>          */
> -       while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
> +       while ((!num || i < num) &&
> +              (data - buf + sizeof(struct dmi_header)) <= len) {
>                 const struct dmi_header *dm = (const struct dmi_header *)data;
>
>                 /*
> @@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(co
>         if (memcmp(buf, "_SM3_", 5) == 0 &&
>             buf[6] < 32 && dmi_checksum(buf, buf[6])) {
>                 dmi_ver = get_unaligned_be16(buf + 7);
> +               dmi_num = 0;                    /* No longer specified */
>                 dmi_len = get_unaligned_le32(buf + 12);
>                 dmi_base = get_unaligned_le64(buf + 16);
>
> -               /*
> -                * The 64-bit SMBIOS 3.0 entry point no longer has a field
> -                * containing the number of structures present in the table.
> -                * Instead, it defines the table size as a maximum size, and
> -                * relies on the end-of-table structure type (#127) to be used
> -                * to signal the end of the table.
> -                * So let's define dmi_num as an upper bound as well: each
> -                * structure has a 4 byte header, so dmi_len / 4 is an upper
> -                * bound for the number of structures in the table.
> -                */
> -               dmi_num = dmi_len / 4;
> -
>                 if (dmi_walk_early(dmi_decode) == 0) {
>                         pr_info("SMBIOS %d.%d present.\n",
>                                 dmi_ver >> 8, dmi_ver & 0xFF);
>
>
> --
> Jean Delvare
> SUSE L3 Support

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
  2015-03-20  8:59 [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow Jean Delvare
  2015-03-20  9:21 ` Ard Biesheuvel
@ 2015-03-26 13:06 ` Matt Fleming
  2015-03-26 13:15   ` Jean Delvare
  1 sibling, 1 reply; 8+ messages in thread
From: Matt Fleming @ 2015-03-26 13:06 UTC (permalink / raw)
  To: Jean Delvare; +Cc: LKML, Matt Fleming, Ard Biesheuvel, Ivan Khoronzhuk

On Fri, 20 Mar, at 09:59:47AM, Jean Delvare wrote:
> dmi_num is a u16, dmi_len is a u32, so this construct:
> 
> 	dmi_num = dmi_len / 4;
> 
> would result in an integer overflow for a DMI table larger than
> 256 kB. I've never see such a large table so far, but SMBIOS 3.0
> makes it possible so maybe we'll see such tables in the future.
> 
> So instead of faking a structure count when the entry point does
> not provide it, adjust the loop condition in dmi_table() to properly
> deal with the case where dmi_num is not set.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/firmware/dmi_scan.c |   22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)

Jean, are you taking this through your tree?

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
  2015-03-26 13:06 ` Matt Fleming
@ 2015-03-26 13:15   ` Jean Delvare
  2015-03-26 14:47     ` Matt Fleming
  0 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2015-03-26 13:15 UTC (permalink / raw)
  To: Matt Fleming; +Cc: LKML, Matt Fleming, Ard Biesheuvel, Ivan Khoronzhuk

Hi Matt,

Le Thursday 26 March 2015 à 13:06 +0000, Matt Fleming a écrit :
> On Fri, 20 Mar, at 09:59:47AM, Jean Delvare wrote:
> > dmi_num is a u16, dmi_len is a u32, so this construct:
> > 
> > 	dmi_num = dmi_len / 4;
> > 
> > would result in an integer overflow for a DMI table larger than
> > 256 kB. I've never see such a large table so far, but SMBIOS 3.0
> > makes it possible so maybe we'll see such tables in the future.
> > 
> > So instead of faking a structure count when the entry point does
> > not provide it, adjust the loop condition in dmi_table() to properly
> > deal with the case where dmi_num is not set.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Matt Fleming <matt.fleming@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> > ---
> >  drivers/firmware/dmi_scan.c |   22 +++++++---------------
> >  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> Jean, are you taking this through your tree?

I don't actually have a tree, so feel free to pick it.

Thanks,
-- 
Jean Delvare
SUSE L3 Support


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
  2015-03-26 13:15   ` Jean Delvare
@ 2015-03-26 14:47     ` Matt Fleming
  2015-03-26 15:21       ` Jean Delvare
  0 siblings, 1 reply; 8+ messages in thread
From: Matt Fleming @ 2015-03-26 14:47 UTC (permalink / raw)
  To: Jean Delvare; +Cc: LKML, Matt Fleming, Ard Biesheuvel, Ivan Khoronzhuk

On Thu, 26 Mar, at 02:15:05PM, Jean Delvare wrote:
> 
> I don't actually have a tree, so feel free to pick it.

OK will do, but this is a regression fix for a potential bug introduced
in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right?

If so, it would be useful for this patch to make reference to that
commit.

How about this?

---

>From 8aabaf128c85bab02fdbe594481826e137c25da4 Mon Sep 17 00:00:00 2001
From: Jean Delvare <jdelvare@suse.de>
Date: Fri, 20 Mar 2015 09:59:47 +0100
Subject: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow

dmi_num is a u16, dmi_len is a u32, so this construct:

	dmi_num = dmi_len / 4;

would result in an integer overflow for a DMI table larger than
256 kB. I've never see such a large table so far, but SMBIOS 3.0
makes it possible so maybe we'll see such tables in the future.

So instead of faking a structure count when the entry point does
not provide it, adjust the loop condition in dmi_table() to properly
deal with the case where dmi_num is not set.

This potential overflow was introduced in commit 6d9ff4733172
("firmware: dmi_scan: Fix dmi_len type").

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Cc: <stable@vger.kernel.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/firmware/dmi_scan.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 69fac068669f..2eebd28b4c40 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len, int num,
 	int i = 0;
 
 	/*
-	 *	Stop when we see all the items the table claimed to have
-	 *	OR we run off the end of the table (also happens)
+	 * Stop when we have seen all the items the table claimed to have
+	 * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run
+	 * off the end of the table (should never happen but sometimes does
+	 * on bogus implementations.)
 	 */
-	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
+	while ((!num || i < num) &&
+	       (data - buf + sizeof(struct dmi_header)) <= len) {
 		const struct dmi_header *dm = (const struct dmi_header *)data;
 
 		/*
@@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(const u8 *buf)
 	if (memcmp(buf, "_SM3_", 5) == 0 &&
 	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
 		dmi_ver = get_unaligned_be16(buf + 7);
+		dmi_num = 0;			/* No longer specified */
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
 
-		/*
-		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
-		 * containing the number of structures present in the table.
-		 * Instead, it defines the table size as a maximum size, and
-		 * relies on the end-of-table structure type (#127) to be used
-		 * to signal the end of the table.
-		 * So let's define dmi_num as an upper bound as well: each
-		 * structure has a 4 byte header, so dmi_len / 4 is an upper
-		 * bound for the number of structures in the table.
-		 */
-		dmi_num = dmi_len / 4;
-
 		if (dmi_walk_early(dmi_decode) == 0) {
 			pr_info("SMBIOS %d.%d present.\n",
 				dmi_ver >> 8, dmi_ver & 0xFF);
-- 
2.1.0

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
  2015-03-26 14:47     ` Matt Fleming
@ 2015-03-26 15:21       ` Jean Delvare
  2015-03-27 12:12         ` Matt Fleming
  0 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2015-03-26 15:21 UTC (permalink / raw)
  To: Matt Fleming; +Cc: LKML, Matt Fleming, Ard Biesheuvel, Ivan Khoronzhuk

Le Thursday 26 March 2015 à 14:47 +0000, Matt Fleming a écrit :
> On Thu, 26 Mar, at 02:15:05PM, Jean Delvare wrote:
> > 
> > I don't actually have a tree, so feel free to pick it.
> 
> OK will do, but this is a regression fix for a potential bug introduced
> in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right?

Not really. I'd rather say it complements commit 6d9ff4733172. The
actual bugs were introduced in commit fc43026278b2. But you can't really
call it a regression as SMBIOS 3.0 entry points were not supported at
all before that commit.

Still a reference to commit fc43026278b2 would certainly be appropriate,
as anyone backporting that commit will also want to pick the two
aforementioned fix-ups (plus ce204e9a4bd8 for that matter.)

Thanks,
Jean

> If so, it would be useful for this patch to make reference to that
> commit.
> 
> How about this?
> 
> ---
> 
> From 8aabaf128c85bab02fdbe594481826e137c25da4 Mon Sep 17 00:00:00 2001
> From: Jean Delvare <jdelvare@suse.de>
> Date: Fri, 20 Mar 2015 09:59:47 +0100
> Subject: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
> 
> dmi_num is a u16, dmi_len is a u32, so this construct:
> 
> 	dmi_num = dmi_len / 4;
> 
> would result in an integer overflow for a DMI table larger than
> 256 kB. I've never see such a large table so far, but SMBIOS 3.0
> makes it possible so maybe we'll see such tables in the future.
> 
> So instead of faking a structure count when the entry point does
> not provide it, adjust the loop condition in dmi_table() to properly
> deal with the case where dmi_num is not set.
> 
> This potential overflow was introduced in commit 6d9ff4733172
> ("firmware: dmi_scan: Fix dmi_len type").
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Matt Fleming <matt.fleming@intel.com>
> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Cc: <stable@vger.kernel.org>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  drivers/firmware/dmi_scan.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 69fac068669f..2eebd28b4c40 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len, int num,
>  	int i = 0;
>  
>  	/*
> -	 *	Stop when we see all the items the table claimed to have
> -	 *	OR we run off the end of the table (also happens)
> +	 * Stop when we have seen all the items the table claimed to have
> +	 * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run
> +	 * off the end of the table (should never happen but sometimes does
> +	 * on bogus implementations.)
>  	 */
> -	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
> +	while ((!num || i < num) &&
> +	       (data - buf + sizeof(struct dmi_header)) <= len) {
>  		const struct dmi_header *dm = (const struct dmi_header *)data;
>  
>  		/*
> @@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(const u8 *buf)
>  	if (memcmp(buf, "_SM3_", 5) == 0 &&
>  	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
>  		dmi_ver = get_unaligned_be16(buf + 7);
> +		dmi_num = 0;			/* No longer specified */
>  		dmi_len = get_unaligned_le32(buf + 12);
>  		dmi_base = get_unaligned_le64(buf + 16);
>  
> -		/*
> -		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
> -		 * containing the number of structures present in the table.
> -		 * Instead, it defines the table size as a maximum size, and
> -		 * relies on the end-of-table structure type (#127) to be used
> -		 * to signal the end of the table.
> -		 * So let's define dmi_num as an upper bound as well: each
> -		 * structure has a 4 byte header, so dmi_len / 4 is an upper
> -		 * bound for the number of structures in the table.
> -		 */
> -		dmi_num = dmi_len / 4;
> -
>  		if (dmi_walk_early(dmi_decode) == 0) {
>  			pr_info("SMBIOS %d.%d present.\n",
>  				dmi_ver >> 8, dmi_ver & 0xFF);
> -- 
> 2.1.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
  2015-03-26 15:21       ` Jean Delvare
@ 2015-03-27 12:12         ` Matt Fleming
  2015-03-27 13:22           ` Jean Delvare
  0 siblings, 1 reply; 8+ messages in thread
From: Matt Fleming @ 2015-03-27 12:12 UTC (permalink / raw)
  To: Jean Delvare; +Cc: LKML, Matt Fleming, Ard Biesheuvel, Ivan Khoronzhuk

On Thu, 26 Mar, at 04:21:53PM, Jean Delvare wrote:
> Le Thursday 26 March 2015 à 14:47 +0000, Matt Fleming a écrit :
> > On Thu, 26 Mar, at 02:15:05PM, Jean Delvare wrote:
> > > 
> > > I don't actually have a tree, so feel free to pick it.
> > 
> > OK will do, but this is a regression fix for a potential bug introduced
> > in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right?
> 
> Not really. I'd rather say it complements commit 6d9ff4733172. The
> actual bugs were introduced in commit fc43026278b2. But you can't really
> call it a regression as SMBIOS 3.0 entry points were not supported at
> all before that commit.
 
OK, thanks for clarifying.

> Still a reference to commit fc43026278b2 would certainly be appropriate,
> as anyone backporting that commit will also want to pick the two
> aforementioned fix-ups (plus ce204e9a4bd8 for that matter.)

commit ce204e9a4bd8 and 6d9ff4733172 are already tagged for stable, so
we should be good there.

This is what I've got queued up to send to tip today. Feel free to send
me another version today if you want to word things differently.

---

>From bfbaafae8519d82d10da6abe75f5766dd5b20475 Mon Sep 17 00:00:00 2001
From: Jean Delvare <jdelvare@suse.de>
Date: Fri, 20 Mar 2015 09:59:47 +0100
Subject: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow

dmi_num is a u16, dmi_len is a u32, so this construct:

	dmi_num = dmi_len / 4;

would result in an integer overflow for a DMI table larger than
256 kB. I've never see such a large table so far, but SMBIOS 3.0
makes it possible so maybe we'll see such tables in the future.

So instead of faking a structure count when the entry point does
not provide it, adjust the loop condition in dmi_table() to properly
deal with the case where dmi_num is not set.

This bug was introduced with the initial SMBIOS 3.0 support in commit
fc43026278b2 ("dmi: add support for SMBIOS 3.0 64-bit entry point").

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Matt Fleming <matt.fleming@intel.com>
Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Cc: <stable@vger.kernel.org>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 drivers/firmware/dmi_scan.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 69fac068669f..2eebd28b4c40 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -86,10 +86,13 @@ static void dmi_table(u8 *buf, u32 len, int num,
 	int i = 0;
 
 	/*
-	 *	Stop when we see all the items the table claimed to have
-	 *	OR we run off the end of the table (also happens)
+	 * Stop when we have seen all the items the table claimed to have
+	 * (SMBIOS < 3.0 only) OR we reach an end-of-table marker OR we run
+	 * off the end of the table (should never happen but sometimes does
+	 * on bogus implementations.)
 	 */
-	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
+	while ((!num || i < num) &&
+	       (data - buf + sizeof(struct dmi_header)) <= len) {
 		const struct dmi_header *dm = (const struct dmi_header *)data;
 
 		/*
@@ -529,21 +532,10 @@ static int __init dmi_smbios3_present(const u8 *buf)
 	if (memcmp(buf, "_SM3_", 5) == 0 &&
 	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
 		dmi_ver = get_unaligned_be16(buf + 7);
+		dmi_num = 0;			/* No longer specified */
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
 
-		/*
-		 * The 64-bit SMBIOS 3.0 entry point no longer has a field
-		 * containing the number of structures present in the table.
-		 * Instead, it defines the table size as a maximum size, and
-		 * relies on the end-of-table structure type (#127) to be used
-		 * to signal the end of the table.
-		 * So let's define dmi_num as an upper bound as well: each
-		 * structure has a 4 byte header, so dmi_len / 4 is an upper
-		 * bound for the number of structures in the table.
-		 */
-		dmi_num = dmi_len / 4;
-
 		if (dmi_walk_early(dmi_decode) == 0) {
 			pr_info("SMBIOS %d.%d present.\n",
 				dmi_ver >> 8, dmi_ver & 0xFF);
-- 
2.1.0

-- 
Matt Fleming, Intel Open Source Technology Center

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow
  2015-03-27 12:12         ` Matt Fleming
@ 2015-03-27 13:22           ` Jean Delvare
  0 siblings, 0 replies; 8+ messages in thread
From: Jean Delvare @ 2015-03-27 13:22 UTC (permalink / raw)
  To: Matt Fleming; +Cc: LKML, Matt Fleming, Ard Biesheuvel, Ivan Khoronzhuk

Hi Matt,

Le Friday 27 March 2015 à 12:12 +0000, Matt Fleming a écrit :
> On Thu, 26 Mar, at 04:21:53PM, Jean Delvare wrote:
> > Le Thursday 26 March 2015 à 14:47 +0000, Matt Fleming a écrit :
> > > On Thu, 26 Mar, at 02:15:05PM, Jean Delvare wrote:
> > > > 
> > > > I don't actually have a tree, so feel free to pick it.
> > > 
> > > OK will do, but this is a regression fix for a potential bug introduced
> > > in commit 6d9ff4733172 ("firmware: dmi_scan: Fix dmi_len type"), right?
> > 
> > Not really. I'd rather say it complements commit 6d9ff4733172. The
> > actual bugs were introduced in commit fc43026278b2. But you can't really
> > call it a regression as SMBIOS 3.0 entry points were not supported at
> > all before that commit.
>  
> OK, thanks for clarifying.
> 
> > Still a reference to commit fc43026278b2 would certainly be appropriate,
> > as anyone backporting that commit will also want to pick the two
> > aforementioned fix-ups (plus ce204e9a4bd8 for that matter.)
> 
> commit ce204e9a4bd8 and 6d9ff4733172 are already tagged for stable, so
> we should be good there.
> 
> This is what I've got queued up to send to tip today. Feel free to send
> me another version today if you want to word things differently.

Looks good, thanks for taking care of it.

-- 
Jean Delvare
SUSE L3 Support


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-03-27 13:22 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-20  8:59 [PATCH] firmware: dmi_scan: Prevent dmi_num integer overflow Jean Delvare
2015-03-20  9:21 ` Ard Biesheuvel
2015-03-26 13:06 ` Matt Fleming
2015-03-26 13:15   ` Jean Delvare
2015-03-26 14:47     ` Matt Fleming
2015-03-26 15:21       ` Jean Delvare
2015-03-27 12:12         ` Matt Fleming
2015-03-27 13:22           ` Jean Delvare

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.