[PATCH 1/2] dmi_scan: Fix missing check for _DMI_ signature in smbios_present()

[PATCH 1/2] dmi_scan: Fix missing check for _DMI_ signature in smbios_present()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

Commit 9f9c9cbb6057 ('drivers/firmware/dmi_scan.c: fetch dmi version
from SMBIOS if it exists') hoisted the check for "_DMI_" into
dmi_scan_machine(), which means that we don't bother to check for
"_DMI_" at offset 16 in an SMBIOS entry.  smbios_present() may also
call dmi_present() for an address where we found "_SM_", if it failed
further validation.

Check for "_DMI_" in smbios_present() before calling dmi_present().

Reported-by: Tim McGrath <tmhikaru@gmail.com>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: stable <stable@vger.kernel.org>
---
This has a memcmp() that wasn't in the previous version, so I've not
included the Acked-by or Tested-by for that.

Ben.

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

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 982f1f5..a86ccff 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -442,7 +442,6 @@ static int __init dmi_present(const char __iomem *p)
 static int __init smbios_present(const char __iomem *p)
 {
 	u8 buf[32];
-	int offset = 0;
 
 	memcpy_fromio(buf, p, 32);
 	if ((buf[5] < 32) && dmi_checksum(buf, buf[5])) {
@@ -461,9 +460,9 @@ static int __init smbios_present(const char __iomem *p)
 			dmi_ver = 0x0206;
 			break;
 		}
-		offset = 16;
+		return memcmp(q + 16, "_DMI_", 5) || dmi_present(p + 16);
 	}
-	return dmi_present(buf + offset);
+	return 1;
 }
 
 void __init dmi_scan_machine(void)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH 2/2] dmi_scan: Refactor dmi_scan_machine(), {smbios,dmi}_present()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 4492 bytes --]

Move the calls to memcpy_fromio() up into the loop in
dmi_scan_machine(), and move the signature checks back down into
dmi_decode().  We need to check at 16-byte intervals but keep a
32-byte buffer for an SMBIOS entry, so shift the buffer after each
iteration.

Merge smbios_present() into dmi_present(), so we look for an SMBIOS
signature at the beginning of the given buffer and then for a DMI
signature at an offset of 16 bytes.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This file mixes up __iomem and regular pointers a lot, and this patch
fixes some but not all of those instances.  Presumably it is quite
safe to read a BIOS image with any mov instructions the compiler
generates, but in that case maybe we should explicitly cast away the
__iomem qualifier in dmi_ioremap()?

Tim, you might like to test that this doesn't cause a regression
of the previous fix.

Ben.

 drivers/firmware/dmi_scan.c |   80 ++++++++++++++++++++-----------------------
 1 file changed, 37 insertions(+), 43 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index a86ccff..3439f59 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -410,22 +410,45 @@ static void __init dmi_dump_ids(void)
 	printk(KERN_CONT "\n");
 }
 
-static int __init dmi_present(const char __iomem *p)
+static int __init dmi_present(const char *buf)
 {
-	u8 buf[15];
+	int smbios_ver;
 
-	memcpy_fromio(buf, p, 15);
-	if (dmi_checksum(buf, 15)) {
+	if (memcmp(buf, "_SM_", 4) == 0 &&
+	    buf[5] < 32 && dmi_checksum(buf, buf[5])) {
+		smbios_ver = (buf[6] << 8) + buf[7];
+
+		/* Some BIOS report weird SMBIOS version, fix that up */
+		switch (smbios_ver) {
+		case 0x021F:
+		case 0x0221:
+			pr_debug("SMBIOS version fixup(2.%d->2.%d)\n",
+				 smbios_ver & 0xFF, 3);
+			smbios_ver = 0x0203;
+			break;
+		case 0x0233:
+			pr_debug("SMBIOS version fixup(2.%d->2.%d)\n", 51, 6);
+			smbios_ver = 0x0206;
+			break;
+		}
+	} else {
+		smbios_ver = 0;
+	}
+
+	buf += 16;
+
+	if (memcmp(buf, "_DMI_", 5) == 0 && dmi_checksum(buf, 15)) {
 		dmi_num = (buf[13] << 8) | buf[12];
 		dmi_len = (buf[7] << 8) | buf[6];
 		dmi_base = (buf[11] << 24) | (buf[10] << 16) |
 			(buf[9] << 8) | buf[8];
 
 		if (dmi_walk_early(dmi_decode) == 0) {
-			if (dmi_ver)
+			if (smbios_ver) {
+				dmi_ver = smbios_ver;
 				pr_info("SMBIOS %d.%d present.\n",
 				       dmi_ver >> 8, dmi_ver & 0xFF);
-			else {
+			} else {
 				dmi_ver = (buf[14] & 0xF0) << 4 |
 					   (buf[14] & 0x0F);
 				pr_info("Legacy DMI %d.%d present.\n",
@@ -435,40 +458,14 @@ static int __init dmi_present(const char __iomem *p)
 			return 0;
 		}
 	}
-	dmi_ver = 0;
-	return 1;
-}
 
-static int __init smbios_present(const char __iomem *p)
-{
-	u8 buf[32];
-
-	memcpy_fromio(buf, p, 32);
-	if ((buf[5] < 32) && dmi_checksum(buf, buf[5])) {
-		dmi_ver = (buf[6] << 8) + buf[7];
-
-		/* Some BIOS report weird SMBIOS version, fix that up */
-		switch (dmi_ver) {
-		case 0x021F:
-		case 0x0221:
-			pr_debug("SMBIOS version fixup(2.%d->2.%d)\n",
-			       dmi_ver & 0xFF, 3);
-			dmi_ver = 0x0203;
-			break;
-		case 0x0233:
-			pr_debug("SMBIOS version fixup(2.%d->2.%d)\n", 51, 6);
-			dmi_ver = 0x0206;
-			break;
-		}
-		return memcmp(q + 16, "_DMI_", 5) || dmi_present(p + 16);
-	}
 	return 1;
 }
 
 void __init dmi_scan_machine(void)
 {
 	char __iomem *p, *q;
-	int rc;
+	char buf[32];
 
 	if (efi_enabled(EFI_CONFIG_TABLES)) {
 		if (efi.smbios == EFI_INVALID_TABLE_ADDR)
@@ -481,10 +478,10 @@ void __init dmi_scan_machine(void)
 		p = dmi_ioremap(efi.smbios, 32);
 		if (p == NULL)
 			goto error;
-
-		rc = smbios_present(p);
+		memcpy_fromio(buf, p, 32);
 		dmi_iounmap(p, 32);
-		if (!rc) {
+
+		if (!dmi_present(buf)) {
 			dmi_available = 1;
 			goto out;
 		}
@@ -499,18 +496,15 @@ void __init dmi_scan_machine(void)
 		if (p == NULL)
 			goto error;
 
+		memset(buf, 0, 16);
 		for (q = p; q < p + 0x10000; q += 16) {
-			if (memcmp(q, "_SM_", 4) == 0 && q - p <= 0xFFE0)
-				rc = smbios_present(q);
-			else if (memcmp(q, "_DMI_", 5) == 0)
-				rc = dmi_present(q);
-			else
-				continue;
-			if (!rc) {
+			memcpy_fromio(buf + 16, q, 16);
+			if (!dmi_present(buf)) {
 				dmi_available = 1;
 				dmi_iounmap(p, 0x10000);
 				goto out;
 			}
+			memcpy(buf, buf + 16, 16);
 		}
 		dmi_iounmap(p, 0x10000);
 	}


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] dmi_scan: Refactor dmi_scan_machine(), {smbios,dmi}_present()

[tmhikaru]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

On Sat, Feb 16, 2013 at 06:02:22PM +0000, Ben Hutchings wrote:

> Tim, you might like to test that this doesn't cause a regression
> of the previous fix.
> 
> Ben.

Ugh, I see what happened now. I only got one copy of this email which was
'helpfully' sorted into the linux kernel mailbox. gmail really doesn't help
with things like this, sorry Ben, I'll test this as soon as I can.

Tim Mcgrath

[-- Attachment #2: Type: application/pgp-signature, Size: 482 bytes --]

Re: [PATCH 2/2] dmi_scan: Refactor dmi_scan_machine(), {smbios,dmi}_present()

[tmhikaru]

[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]

On Sat, Feb 16, 2013 at 06:02:22PM +0000, Ben Hutchings wrote:
> Move the calls to memcpy_fromio() up into the loop in
> dmi_scan_machine(), and move the signature checks back down into
> dmi_decode().  We need to check at 16-byte intervals but keep a
> 32-byte buffer for an SMBIOS entry, so shift the buffer after each
> iteration.
> 
> Merge smbios_present() into dmi_present(), so we look for an SMBIOS
> signature at the beginning of the given buffer and then for a DMI
> signature at an offset of 16 bytes.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> This file mixes up __iomem and regular pointers a lot, and this patch
> fixes some but not all of those instances.  Presumably it is quite
> safe to read a BIOS image with any mov instructions the compiler
> generates, but in that case maybe we should explicitly cast away the
> __iomem qualifier in dmi_ioremap()?
> 
> Tim, you might like to test that this doesn't cause a regression
> of the previous fix.

I can confirm that this works just as well as the last patch you gave me.
I'm sorry that it took me a while to reply.

Tim McGrath

[-- Attachment #2: Type: application/pgp-signature, Size: 482 bytes --]

Re: [PATCH 2/2] dmi_scan: Refactor dmi_scan_machine(), {smbios,dmi}_present()

[tmhikaru]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

	Forgive me for bothering you about this again, I know that -stable
has a policy of only accepting patches once Linus has accepted them
upstream, but I'm curious what's going on here. Is there something I could
help with to move this along? I haven't seen any discussion about it since
Feb 16.

	Of course this assumes that I have not missed the patch going in...
Tim McGrath

[-- Attachment #2: Type: application/pgp-signature, Size: 482 bytes --]

Re: [PATCH 2/2] dmi_scan: Refactor dmi_scan_machine(), {smbios,dmi}_present()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

On Mon, 2013-03-04 at 15:09 -0500, tmhikaru@gmail.com wrote:
> 	Forgive me for bothering you about this again, I know that -stable
> has a policy of only accepting patches once Linus has accepted them
> upstream, but I'm curious what's going on here. Is there something I could
> help with to move this along? I haven't seen any discussion about it since
> Feb 16.
> 
> 	Of course this assumes that I have not missed the patch going in...
> Tim McGrath

Andrew, did you see these dmi_scan patches?  Should I repost them?

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] dmi_scan: Fix missing check for _DMI_ signature in smbios_present()

[tmhikaru]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]

On Sat, Feb 16, 2013 at 06:35:04PM -0800, Zhenzhong Duan wrote:
> 
> ----- ben@decadent.org.uk wrote???
> 
> > Commit 9f9c9cbb6057 ('drivers/firmware/dmi_scan.c: fetch dmi version
> > from SMBIOS if it exists') hoisted the check for "_DMI_" into
> > dmi_scan_machine(), which means that we don't bother to check for
> > "_DMI_" at offset 16 in an SMBIOS entry.  smbios_present() may also
> > call dmi_present() for an address where we found "_SM_", if it failed
> > further validation.
> > 
> > Check for "_DMI_" in smbios_present() before calling dmi_present().
> > 
> > Reported-by: Tim McGrath <tmhikaru@gmail.com>
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Cc: stable <stable@vger.kernel.org>
> > ---
> > This has a memcmp() that wasn't in the previous version, so I've not
> > included the Acked-by or Tested-by for that.
> Yes, the further "_DMI_" check is needed.
> Acked-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> > 
> > Ben.
> > 
> >  drivers/firmware/dmi_scan.c |    5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/firmware/dmi_scan.c
> > b/drivers/firmware/dmi_scan.c
> > index 982f1f5..a86ccff 100644
> > --- a/drivers/firmware/dmi_scan.c
> > +++ b/drivers/firmware/dmi_scan.c
> > @@ -442,7 +442,6 @@ static int __init dmi_present(const char __iomem
> > *p)
> >  static int __init smbios_present(const char __iomem *p)
> >  {
> >  	u8 buf[32];
> > -	int offset = 0;
> >  
> >  	memcpy_fromio(buf, p, 32);
> >  	if ((buf[5] < 32) && dmi_checksum(buf, buf[5])) {
> > @@ -461,9 +460,9 @@ static int __init smbios_present(const char
> > __iomem *p)
> >  			dmi_ver = 0x0206;
> >  			break;
> >  		}
> > -		offset = 16;
> > +		return memcmp(q + 16, "_DMI_", 5) || dmi_present(p + 16);
> >  	}
> > -	return dmi_present(buf + offset);
> > +	return 1;
> >  }
> >  
> >  void __init dmi_scan_machine(void)
I'm a little confused, do you need me to test something?
Tim McGrath

[-- Attachment #2: Type: application/pgp-signature, Size: 482 bytes --]

Re: [PATCH 1/2] dmi_scan: Fix missing check for _DMI_ signature in smbios_present()

[Zhenzhong Duan]

----- ben@decadent.org.uk wrote：

> Commit 9f9c9cbb6057 ('drivers/firmware/dmi_scan.c: fetch dmi version
> from SMBIOS if it exists') hoisted the check for "_DMI_" into
> dmi_scan_machine(), which means that we don't bother to check for
> "_DMI_" at offset 16 in an SMBIOS entry.  smbios_present() may also
> call dmi_present() for an address where we found "_SM_", if it failed
> further validation.
> 
> Check for "_DMI_" in smbios_present() before calling dmi_present().
> 
> Reported-by: Tim McGrath <tmhikaru@gmail.com>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: stable <stable@vger.kernel.org>
> ---
> This has a memcmp() that wasn't in the previous version, so I've not
> included the Acked-by or Tested-by for that.
Yes, the further "_DMI_" check is needed.
Acked-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> 
> Ben.
> 
>  drivers/firmware/dmi_scan.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c
> b/drivers/firmware/dmi_scan.c
> index 982f1f5..a86ccff 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -442,7 +442,6 @@ static int __init dmi_present(const char __iomem
> *p)
>  static int __init smbios_present(const char __iomem *p)
>  {
>  	u8 buf[32];
> -	int offset = 0;
>  
>  	memcpy_fromio(buf, p, 32);
>  	if ((buf[5] < 32) && dmi_checksum(buf, buf[5])) {
> @@ -461,9 +460,9 @@ static int __init smbios_present(const char
> __iomem *p)
>  			dmi_ver = 0x0206;
>  			break;
>  		}
> -		offset = 16;
> +		return memcmp(q + 16, "_DMI_", 5) || dmi_present(p + 16);
>  	}
> -	return dmi_present(buf + offset);
> +	return 1;
>  }
>  
>  void __init dmi_scan_machine(void)