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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On 11 February 2015 at 17:46, Ivan Khoronzhuk
<[email protected]> 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 <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
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
>
On 11 February 2015 at 17:46, Ivan Khoronzhuk
<[email protected]> 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 <[email protected]>
> ---
> 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
>
On 02/11/2015 11:55 AM, Ard Biesheuvel wrote:
> On 11 February 2015 at 17:46, Ivan Khoronzhuk
> <[email protected]> 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.
On 02/11/2015 11:53 AM, Ard Biesheuvel wrote:
> On 11 February 2015 at 17:46, Ivan Khoronzhuk
> <[email protected]> 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 <[email protected]>
> Acked-by: Ard Biesheuvel <[email protected]>
>
> This should get a cc stable as well.
>
Pay attention that this patch has to be applied with patch 1/3.
On 11 February 2015 at 18:10, Ivan Khoronzhuk
<[email protected]> wrote:
>
> On 02/11/2015 11:53 AM, Ard Biesheuvel wrote:
>>
>> On 11 February 2015 at 17:46, Ivan Khoronzhuk
>> <[email protected]> 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 <[email protected]>
>>
>> Acked-by: Ard Biesheuvel <[email protected]>
>>
>> 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?
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
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.