2009-06-06 22:03:07

by Alan

[permalink] [raw]
Subject: [PATCH] dmi: sanity check BIOS tables

From: Alan Cox <[email protected]>

There are various places where invalid DMI tables cause "undefined" kernel
behaviour at boot. Be more robust.

Signed-off-by: Alan Cox <[email protected]>
---

drivers/firmware/dmi_scan.c | 34 +++++++++++++++++++++++++++++-----
1 files changed, 29 insertions(+), 5 deletions(-)


diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 5f1b540..0bc0034 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -20,21 +20,32 @@ static char dmi_empty_string[] = " ";
*/
static int dmi_initialized;

+static u8 *dmi_end;
+
static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
{
const u8 *bp = ((u8 *) dm) + dm->length;

if (s) {
s--;
- while (s > 0 && *bp) {
+ while (s > 0) {
+ /* Catch corrupt table ends before we walk into
+ undefined mappings */
+ if (bp >= dmi_end)
+ goto invalid;
+ if (*bp == 0)
+ break;
bp += strlen(bp) + 1;
s--;
}

if (*bp != 0) {
- size_t len = strlen(bp)+1;
+ /* Use strnlen in case we have a BIOS table error */
+ size_t len = strnlen(bp, dmi_end - bp) + 1;
size_t cmp_len = len > 8 ? 8 : len;

+ if (bp == dmi_end)
+ goto invalid;
if (!memcmp(bp, dmi_empty_string, cmp_len))
return dmi_empty_string;
return bp;
@@ -42,6 +53,9 @@ static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
}

return "";
+invalid:
+ printk(KERN_ERR "dmi: table overrun - corrupt DMI tables ?\n");
+ return dmi_empty_string;
}

static char * __init dmi_string(const struct dmi_header *dm, u8 s)
@@ -109,9 +123,11 @@ static int __init dmi_walk_early(void (*decode)(const struct dmi_header *,
if (buf == NULL)
return -1;

+ /* Remember the end mark for string walking */
+ dmi_end = buf + dmi_len;
dmi_table(buf, dmi_len, dmi_num, decode, NULL);
-
dmi_iounmap(buf, dmi_len);
+
return 0;
}

@@ -154,7 +170,7 @@ static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, int inde
char *s;
int is_ff = 1, is_00 = 1, i;

- if (dmi_ident[slot])
+ if (dmi_ident[slot] || d > dmi_end - 16)
return;

for (i = 0; i < 16 && (is_ff || is_00); i++) {
@@ -182,7 +198,7 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot, int inde
const u8 *d = (u8*) dm + index;
char *s;

- if (dmi_ident[slot])
+ if (dmi_ident[slot] || d == dmi_end)
return;

s = dmi_alloc(4);
@@ -260,6 +276,11 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
struct dmi_device *dev;
void * data;

+ if (((u8 *)dm) + dm->length > dmi_end) {
+ printk(KERN_ERR "dmi_save_ipmi_device: table overrun.\n");
+ return;
+ }
+
data = dmi_alloc(dm->length);
if (data == NULL) {
printk(KERN_ERR "dmi_save_ipmi_device: out of memory.\n");
@@ -285,6 +306,9 @@ static void __init dmi_save_extended_devices(const struct dmi_header *dm)
{
const u8 *d = (u8*) dm + 5;

+ if (d >= dmi_end)
+ return;
+
/* Skip disabled device */
if ((*d & 0x80) == 0)
return;


2009-06-07 08:57:37

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] dmi: sanity check BIOS tables

On 06/07/2009 01:01 AM, Alan Cox wrote:
> @@ -260,6 +276,11 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
> struct dmi_device *dev;
> void * data;
>
> + if (((u8 *)dm) + dm->length > dmi_end) {

I think this should be >= too.

> + printk(KERN_ERR "dmi_save_ipmi_device: table overrun.\n");
> + return;
> + }
> +

2009-06-07 13:56:26

by Jeff Mitchell

[permalink] [raw]
Subject: Re: [PATCH] dmi: sanity check BIOS tables

On Sun, Jun 7, 2009 at 4:57 AM, Jiri Slaby<[email protected]> wrote:
> On 06/07/2009 01:01 AM, Alan Cox wrote:
>> @@ -260,6 +276,11 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
>>       struct dmi_device *dev;
>>       void * data;
>>
>> +     if (((u8 *)dm) + dm->length > dmi_end) {
>
> I think this should be >= too.
>
>> +             printk(KERN_ERR "dmi_save_ipmi_device: table overrun.\n");
>> +             return;
>> +     }
>> +

I've tried this patch, both with Jiri's suggestion and without.
Neither makes a difference, except for slightly changing the output (
new output at http://tinypic.com/r/2di4mlg/5 )

Let me know if I can provide anything more to help debug.

Thanks,
Jeff

2009-06-07 19:08:53

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] dmi: sanity check BIOS tables

On 06/07/2009 03:56 PM, Jeff Mitchell wrote:
> Let me know if I can provide anything more to help debug.

It dies on DMI_BIOS_VERSION. I don't see it, maybe I'm blind, could you
provide
dmidecode -u -t 0
?

2009-06-08 02:08:58

by Jeff Mitchell

[permalink] [raw]
Subject: Re: [PATCH] dmi: sanity check BIOS tables

On Sun, Jun 7, 2009 at 3:08 PM, Jiri Slaby<[email protected]> wrote:
> On 06/07/2009 03:56 PM, Jeff Mitchell wrote:
>> Let me know if I can provide anything more to help debug.
>
> It dies on DMI_BIOS_VERSION. I don't see it, maybe I'm blind, could you
> provide
> dmidecode -u -t 0
> ?
>

Sure. It's below. Thanks, again.

# dmidecode 2.9
SMBIOS 2.31 present.

Handle 0x0000, DMI type 0, 20 bytes
Header and Data:
00 14 00 00 01 02 91 E7 03 00 90 5F 00 00 00 00
00 00 82 01
Strings:
50 68 6F 65 6E 69 78 20 54 65 63 68 6E 6F 6C 6F
67 69 65 73 20 4C 54 44 00
"Phoenix Technologies LTD"
36 2E 30 30 00
"6.00"
30 31 2F 33 30 2F 32 30 30 38 00
"01/30/2008"