[Patch 0/3] firmware: dmi_scan: add some SMBIOSv3 corrections

[Patch 0/3] firmware: dmi_scan: add some SMBIOSv3 corrections

[Ivan Khoronzhuk]

This series adds corrections to dmi_scan:
- extends version to be like 3.4.5 format
- fix dmi_len to be 32 bit wide

Ivan Khoronzhuk (3):
  firmware: dmi_scan: use direct access to static vars
  firmware: dmi_scan: fix dmi_len type
  firmware: dmi_scan: use full dmi version for SMBIOS3

 drivers/firmware/dmi_scan.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

-- 
1.9.1

[Patch 1/3] firmware: dmi_scan: use direct access to static vars

[Ivan Khoronzhuk]

There is no reason to pass static vars to function that can use
only them.

The dmi_table() can use only dmi_len and dmi_num static vars, so use
them directly. In this case we can freely change their type in one
place and slightly decrease redundancy.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/firmware/dmi_scan.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index d55c712..fb16203 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -18,6 +18,8 @@
 static const char dmi_empty_string[] = "        ";
 
 static u16 __initdata dmi_ver;
+static u16 dmi_len;
+static u16 dmi_num;
 /*
  * Catch too early calls to dmi_check_system():
  */
@@ -78,7 +80,7 @@ static const char * __init dmi_string(const struct dmi_header *dm, u8 s)
  *	We have to be cautious here. We have seen BIOSes with DMI pointers
  *	pointing to completely the wrong place for example
  */
-static void dmi_table(u8 *buf, int len, int num,
+static void dmi_table(u8 *buf,
 		      void (*decode)(const struct dmi_header *, void *),
 		      void *private_data)
 {
@@ -89,7 +91,8 @@ static void dmi_table(u8 *buf, int len, int num,
 	 *	Stop when we see all the items the table claimed to have
 	 *	OR we run off the end of the table (also happens)
 	 */
-	while ((i < num) && (data - buf + sizeof(struct dmi_header)) <= len) {
+	while ((i < dmi_num) && (data - buf + sizeof(struct dmi_header))
+		<= dmi_len) {
 		const struct dmi_header *dm = (const struct dmi_header *)data;
 
 		/*
@@ -104,9 +107,9 @@ static void dmi_table(u8 *buf, int len, int num,
 		 *  table in dmi_decode or dmi_string
 		 */
 		data += dm->length;
-		while ((data - buf < len - 1) && (data[0] || data[1]))
+		while ((data - buf < dmi_len - 1) && (data[0] || data[1]))
 			data++;
-		if (data - buf < len - 1)
+		if (data - buf < dmi_len - 1)
 			decode(dm, private_data);
 		data += 2;
 		i++;
@@ -116,8 +119,6 @@ static void dmi_table(u8 *buf, int len, int num,
 static u8 smbios_header[32];
 static int smbios_header_size;
 static phys_addr_t dmi_base;
-static u16 dmi_len;
-static u16 dmi_num;
 
 static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 		void *))
@@ -128,7 +129,7 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
 	if (buf == NULL)
 		return -1;
 
-	dmi_table(buf, dmi_len, dmi_num, decode, NULL);
+	dmi_table(buf, decode, NULL);
 
 	add_device_randomness(buf, dmi_len);
 
@@ -908,7 +909,7 @@ int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 	if (buf == NULL)
 		return -1;
 
-	dmi_table(buf, dmi_len, dmi_num, decode, private_data);
+	dmi_table(buf, decode, private_data);
 
 	dmi_unmap(buf);
 	return 0;
-- 
1.9.1

[Patch 2/3] firmware: dmi_scan: fix dmi_len type

[Ivan Khoronzhuk]

According to SMBIOSv3 specification the length of DMI table can be
up to 32bits wide. So use appropriate type to avoid overflow.

It's obvious that dmi_num theoretically can be more than u16 also,
so it's can be changed to u32 or at least it's better to use int
instead of u16, but on that moment I cannot imagine dmi structure
count more than 65535 and it can require changing type of vars that
work with it. So I didn't correct it.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/firmware/dmi_scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index fb16203..952e95c 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -18,7 +18,7 @@
 static const char dmi_empty_string[] = "        ";
 
 static u16 __initdata dmi_ver;
-static u16 dmi_len;
+static u32 dmi_len;
 static u16 dmi_num;
 /*
  * Catch too early calls to dmi_check_system():
-- 
1.9.1

[Patch 3/3] firmware: dmi_scan: use full dmi version for SMBIOS3

[Ivan Khoronzhuk]

New SMBIOS3 spec adds additional field for versioning - docrev.
The docrev identifies the revision of a specification implemented in
the table structures, so display SMBIOS version > 3 in format,
like: 3.22.1

It's not affect on other part of code because version number
is analyzed using comparing, and it's obvious that 0x000208 is less
than 0x030201 for example.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
 drivers/firmware/dmi_scan.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 952e95c..e4a2d25 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -17,7 +17,7 @@
  */
 static const char dmi_empty_string[] = "        ";
 
-static u16 __initdata dmi_ver;
+static u32 dmi_ver __initdata;
 static u32 dmi_len;
 static u16 dmi_num;
 /*
@@ -534,7 +534,8 @@ 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_ver = get_unaligned_be32(buf + 6);
+		dmi_ver &= 0xFFFFFF;
 		dmi_len = get_unaligned_le32(buf + 12);
 		dmi_base = get_unaligned_le64(buf + 16);
 		smbios_header_size = buf[6];
@@ -553,8 +554,9 @@ static int __init dmi_smbios3_present(const u8 *buf)
 		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);
+			pr_info("SMBIOS %d.%d.%d present.\n",
+				dmi_ver >> 16, (dmi_ver >> 8) & 0xFF,
+				dmi_ver & 0xFF);
 			dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
 			pr_debug("DMI: %s\n", dmi_ids_string);
 			return 0;
-- 
1.9.1

Re: [Patch 2/3] firmware: dmi_scan: fix dmi_len type

[Ard Biesheuvel]

On 11 February 2015 at 17:46, Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
> According to SMBIOSv3 specification the length of DMI table can be
> up to 32bits wide. So use appropriate type to avoid overflow.
>
> It's obvious that dmi_num theoretically can be more than u16 also,
> so it's can be changed to u32 or at least it's better to use int
> instead of u16, but on that moment I cannot imagine dmi structure
> count more than 65535 and it can require changing type of vars that
> work with it. So I didn't correct it.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

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

This should get a cc stable as well.

-- 
Ard.

> ---
>  drivers/firmware/dmi_scan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index fb16203..952e95c 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -18,7 +18,7 @@
>  static const char dmi_empty_string[] = "        ";
>
>  static u16 __initdata dmi_ver;
> -static u16 dmi_len;
> +static u32 dmi_len;
>  static u16 dmi_num;
>  /*
>   * Catch too early calls to dmi_check_system():
> --
> 1.9.1
>

Re: [Patch 3/3] firmware: dmi_scan: use full dmi version for SMBIOS3

[Ard Biesheuvel]

On 11 February 2015 at 17:46, Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
> New SMBIOS3 spec adds additional field for versioning - docrev.
> The docrev identifies the revision of a specification implemented in
> the table structures, so display SMBIOS version > 3 in format,
> like: 3.22.1
>
> It's not affect on other part of code because version number
> is analyzed using comparing, and it's obvious that 0x000208 is less
> than 0x030201 for example.
>

I don't think the spec forbids passing a 3.0+ table using the legacy
32-bit entry point, in which case the packing of the version could
potentially become a problem.

I don't care deeply about the docrev, so I think we could drop this
patch, but if others feel differently, could we at least pack the
version in a consistent manner (i.e., always account for docrev, and
set it to 0 if the 32-bit entry point is used?)

-- 
Ard.

> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>  drivers/firmware/dmi_scan.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 952e95c..e4a2d25 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -17,7 +17,7 @@
>   */
>  static const char dmi_empty_string[] = "        ";
>
> -static u16 __initdata dmi_ver;
> +static u32 dmi_ver __initdata;
>  static u32 dmi_len;
>  static u16 dmi_num;
>  /*
> @@ -534,7 +534,8 @@ 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_ver = get_unaligned_be32(buf + 6);
> +               dmi_ver &= 0xFFFFFF;
>                 dmi_len = get_unaligned_le32(buf + 12);
>                 dmi_base = get_unaligned_le64(buf + 16);
>                 smbios_header_size = buf[6];
> @@ -553,8 +554,9 @@ static int __init dmi_smbios3_present(const u8 *buf)
>                 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);
> +                       pr_info("SMBIOS %d.%d.%d present.\n",
> +                               dmi_ver >> 16, (dmi_ver >> 8) & 0xFF,
> +                               dmi_ver & 0xFF);
>                         dmi_format_ids(dmi_ids_string, sizeof(dmi_ids_string));
>                         pr_debug("DMI: %s\n", dmi_ids_string);
>                         return 0;
> --
> 1.9.1
>

Re: [Patch 3/3] firmware: dmi_scan: use full dmi version for SMBIOS3

[Ivan Khoronzhuk]

On 02/11/2015 11:55 AM, Ard Biesheuvel wrote:
> On 11 February 2015 at 17:46, Ivan Khoronzhuk
> <ivan.khoronzhuk@linaro.org> wrote:
>> New SMBIOS3 spec adds additional field for versioning - docrev.
>> The docrev identifies the revision of a specification implemented in
>> the table structures, so display SMBIOS version > 3 in format,
>> like: 3.22.1
>>
>> It's not affect on other part of code because version number
>> is analyzed using comparing, and it's obvious that 0x000208 is less
>> than 0x030201 for example.
>>
> I don't think the spec forbids passing a 3.0+ table using the legacy
> 32-bit entry point, in which case the packing of the version could
> potentially become a problem.
>
> I don't care deeply about the docrev, so I think we could drop this
> patch, but if others feel differently, could we at least pack the
> version in a consistent manner (i.e., always account for docrev, and
> set it to 0 if the 32-bit entry point is used?)
>

If others are OK, I can add docrev for 32 bit 3+ versions.

Re: [Patch 2/3] firmware: dmi_scan: fix dmi_len type

[Ivan Khoronzhuk]

On 02/11/2015 11:53 AM, Ard Biesheuvel wrote:
> On 11 February 2015 at 17:46, Ivan Khoronzhuk
> <ivan.khoronzhuk@linaro.org> wrote:
>> According to SMBIOSv3 specification the length of DMI table can be
>> up to 32bits wide. So use appropriate type to avoid overflow.
>>
>> It's obvious that dmi_num theoretically can be more than u16 also,
>> so it's can be changed to u32 or at least it's better to use int
>> instead of u16, but on that moment I cannot imagine dmi structure
>> count more than 65535 and it can require changing type of vars that
>> work with it. So I didn't correct it.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Acked-by: Ard Biesheuvel <ard@linaro.org>
>
> This should get a cc stable as well.
>

Pay attention that this patch has to be applied with patch 1/3.

Re: [Patch 2/3] firmware: dmi_scan: fix dmi_len type

[Ard Biesheuvel]

On 11 February 2015 at 18:10, Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> On 02/11/2015 11:53 AM, Ard Biesheuvel wrote:
>>
>> On 11 February 2015 at 17:46, Ivan Khoronzhuk
>> <ivan.khoronzhuk@linaro.org> wrote:
>>>
>>> According to SMBIOSv3 specification the length of DMI table can be
>>> up to 32bits wide. So use appropriate type to avoid overflow.
>>>
>>> It's obvious that dmi_num theoretically can be more than u16 also,
>>> so it's can be changed to u32 or at least it's better to use int
>>> instead of u16, but on that moment I cannot imagine dmi structure
>>> count more than 65535 and it can require changing type of vars that
>>> work with it. So I didn't correct it.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>
>> Acked-by: Ard Biesheuvel <ard@linaro.org>
>>
>> This should get a cc stable as well.
>>
>
> Pay attention that this patch has to be applied with patch 1/3.

Good point. Actually, I don't really see the need for patch #1, even
if I agree that it would have been better to write it like you have in
the first place.
But leaving the dmi_len as u16 is clearly a bug on my part, so that
should be fixed.

@Matt: any thoughts?

Re: [Patch 2/3] firmware: dmi_scan: fix dmi_len type

[Matt Fleming]

On Wed, 11 Feb, at 06:12:59PM, Ard Biesheuvel wrote:
> 
> Good point. Actually, I don't really see the need for patch #1, even
> if I agree that it would have been better to write it like you have in
> the first place.
> But leaving the dmi_len as u16 is clearly a bug on my part, so that
> should be fixed.
> 
> @Matt: any thoughts?

Ivan, I'd prefer it if you move PATCH 1 to be PATCH 3, i.e. make the
urgent changes at the beginning of the series and the cleanups at the
end. That nicely sidesteps the issue of having to backport a cleanup
patch as a dependency for a real bug fix.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [Patch 2/3] firmware: dmi_scan: fix dmi_len type

[Ivan Khoronzhuk]

On 02/13/2015 06:12 PM, Matt Fleming wrote:
> On Wed, 11 Feb, at 06:12:59PM, Ard Biesheuvel wrote:
>> Good point. Actually, I don't really see the need for patch #1, even
>> if I agree that it would have been better to write it like you have in
>> the first place.
>> But leaving the dmi_len as u16 is clearly a bug on my part, so that
>> should be fixed.
>>
>> @Matt: any thoughts?
> Ivan, I'd prefer it if you move PATCH 1 to be PATCH 3, i.e. make the
> urgent changes at the beginning of the series and the cleanups at the
> end. That nicely sidesteps the issue of having to backport a cleanup
> patch as a dependency for a real bug fix.
>

Ok.
I'll update soon.