Re: [PATCH 2.2] i386/dmi_scan updates

Re: [PATCH 2.2] i386/dmi_scan updates

[Jean Delvare]

> btw word wrap is broken on your mailer

I'm sorry about that. I have access to no SMTP server here and have to use a webmail client, which does no word wrap at all (and I'm rather happy with that since it allows me to send inline patches without having them totally messed up). I'm doing my best to word wrap quotations by myself but I may fail sometimes.

>> Also note that the white spaces check has been removed
>> from 2.4.
>The debug data can basically go

I'm not sure I get you. The debug data is still present and I think it is a good idea (we can enable it to blacklist systems that wouldn't even boot without an appropriate workaround). Only the white space check was removed. Anyway, I still this this check was bad, as was the null byte check also. See below.

>> A better way IMHO would be to "secure" the dmi_string
>> function. If we can ensure it will always return a safe
>> (that is, null terminated) string, we are done. Agreed?
>I'd ascii filter it as well but yes. The length one I dont
> think is a problem because the table length will gie us a
> defined worst case

I don't agree with ASCII filtering. I don't want to enlarge everyone's kernel for just some rare cases where the DMI table is broken *and* debug code is enabled. If you want, I can write the code that does it, but I wouldn't enable it by default.
As far as the length is concerned, the table length doesn't help, because we check the structure length against the remaining table length. The structure length does *not* include the string data, so we could pass the length test and still run of the table in dmi_string. What's more, the string index could be more that the string count for this structure and no check is done for this.
I think we need a safer dmi_string function that knows about the table length (or, better indeed, the remaining length from this point), and checks for both string index being too large and string index leading outside the table. Then, the other checks (white space and null byte) will be obsolete.

Jean Delvare


___________________________________
Webmail Nerim, http://www.nerim.net/

Re: [PATCH 2.2] i386/dmi_scan updates

[Alan Cox]

On Sun, 2002-10-06 at 13:12, Jean Delvare wrote:
> I don't agree with ASCII filtering. I don't want to enlarge everyone's kernel for just some rare cases where the DMI table is broken *and* debug code is enabled. If you want, I can write the code that does it, but I wouldn't enable it by default.
> As far as the length is concerned, the table length doesn't help, because we check the structure length against the remaining table length. The structure length does *not* include the string data, so we could pass the length test and still run of the table in dmi_string. What's more, the string index could be more that the string count for this structure and no check is done for this.
> I think we need a safer dmi_string function that knows about the table length (or, better indeed, the remaining length from this point), and checks for both string index being too large and string index leading outside the table. Then, the other checks (white space and null byte) will be obsolete.

Our console doesn't handle arbitary 8bit encodings. There are japanese
DMI strings out there for example

Re: [PATCH 2.2] i386/dmi_scan updates

[Alan Cox]

On Sun, 2002-10-06 at 13:12, Jean Delvare wrote
> 
> I don't agree with ASCII filtering. I don't want to enlarge everyone's kernel for just some rare cases where the DMI table is broken *and* debug code is enabled. If you want, I can write the code that does it, but I wouldn't enable it by default.
> As far as the length is concerned, the table length doesn't help, because we check the structure length against the remaining table length. The structure length does *not* include the string data, so we could pass the length test and still run of the table in dmi_string. What's more, the string index could be more that the string count for this structure and no check is done for this.
> I think we need a safer dmi_string function that knows about the table length (or, better indeed, the remaining length from this point), and checks for both string index being too large and string index leading outside the table. Then, the other checks (white space and null byte) will be obsolete.

Oh as a PS btw don't worry about code size for the dmi scanner as it is
all marked __init. The entire DMI code gets turned back into free memory
by the end of the boot of the kernel, so you can put complex checks in
there if it helps

Re: [PATCH 2.2] i386/dmi_scan updates

[Jean Delvare]

> Our console doesn't handle arbitary 8bit encodings. There
> are japanese DMI strings out there for example
OK. Anyway, this is for debugging only and I don't intend to do it right now, if I ever do. I came up to a new patch that I still need to test. I'll be posting it tomorrow if it works as intended. It should solve everything we've been discussing about except ascii filtering.

> Oh as a PS btw don't worry about code size for the dmi
> scanner as it is all marked __init. The entire DMI code
> gets turned back into free memory by the end of the boot
> of the kernel, so you can put complex checks in there if
> it helps
I figured this out right after posting my message. I am really new to kernel code, as you see. I'll remember this, thanks.

Jean Delvare


___________________________________
Webmail Nerim, http://www.nerim.net/

Re: [PATCH 2.2] i386/dmi_scan updates

[Alan Cox]

On Sat, 2002-10-05 at 23:19, Jean Delvare wrote:
> This check has been removed in 2.4 though. I think it was needed when we were 
> trusting the structure count (see version 1.1 of dmidecode) instead of
> also verifying we weren't running of the table. Now that this check is
> done, I don't see why we would need the heuristic anymore.

True - btw word wrap is broken on your mailer

> Also note that the white spaces check has been removed from 2.4.

The debug data can basically go

> A better way IMHO would be to "secure" the dmi_string function. If we can 
ensure it will always return a safe (that is, null terminated) string, we are done. Agreed?

I'd ascii filter it as well but yes. The length one I dont think is a
problem because the table length will gie us a defined worst case

Re: [PATCH 2.2] i386/dmi_scan updates

[Jean Delvare]

>>  - Stop skipping DMI entries when type is less than those of the
>>    previous entry. I could see no reason for doing this.

> Fixes crashes on certain vendors hardware. It shouldnt be
> needed, however in the real world it proves to be a
> rather essential heuristic. Dmidecode doesnt do it
> because in userspace I dont mind spewing crap to show a
> user a problem.

This check has been removed in 2.4 though. I think it was needed when we were trusting the structure count (see version 1.1 of dmidecode) instead of also verifying we weren't running of the table. Now that this check is done, I don't see why we would need the heuristic anymore.

(...)

>>  - Remove senseless tests in dump (debug) code.

> These are also not senseless. Not everyone seems to use
> the proper null string, sometimes you get spaces too

I don't see how the check would protect us against anything. It only looks if the first char is a null byte or a white space. This is not very helpful, since on one hand such strings may be valid, and on the other hand invalid strings may pass the test.

Also note that the white spaces check has been removed from 2.4.

> The technical changes look right, and in theory all of it
> does. In practice I'd rather see a patch that kept the
> rule of thumb about order and the ' ' check

A better way IMHO would be to "secure" the dmi_string function. If we can ensure it will always return a safe (that is, null terminated) string, we are done. Agreed?

Jean Delvare


___________________________________
Webmail Nerim, http://www.nerim.net/

Re: [PATCH 2.2] i386/dmi_scan updates

[Alan Cox]

On Sat, 2002-10-05 at 22:36, Jean Delvare wrote:
>  - Stop skipping DMI entries when type is less than those of the
>    previous entry. I could see no reason for doing this.

Fixes crashes on certain vendors hardware. It shouldnt be needed,
however in the real world it proves to be a rather essential heuristic.
DMidecode doesnt do it because in userspace I dont mind spewing crap to
show a user a problem.

>  - Verify the DMI entry point structure checksum.
>  - Start looking for the DMI entry point from 0xF0000, not 0xE0000.

Looks ok

>  - Fix an off-by-one error causing the last address scanned being
>    0x100000, not 0xFFFF0 as it should.

Yep

>  - Do not display the DMI version if it would be 0.0.
>  - Remove senseless tests in dump (debug) code.

These are also not senseless. Not everyone seems to use the proper null
string, sometimes you get spaces too


The technical changes look right, and in theory all of it does. In
practice I'd rather see a patch that kept the rule of thumb about order
and the ' ' check

[PATCH 2.2] i386/dmi_scan updates

[Jean Delvare]

Hi everyone,

I have been working on Alan's userspace program "dmidecode" recently, and made some fixes to it. It happens that this userspace program shares some code with linux/arch/i386/dmi_scan.c, so I thought it would be a good idea to backport the changes that apply to the linux kernel tree. I have been writing patches for Linux 2.2, 2.4 and 2.5.

This specific patch applies to the Linux 2.2 tree and has been built against 2.2.22.

Main changes:
 - Stop skipping DMI entries when type is less than those of the
   previous entry. I could see no reason for doing this.
 - Verify the DMI entry point structure checksum.
 - Start looking for the DMI entry point from 0xF0000, not 0xE0000.
 - Fix an off-by-one error causing the last address scanned being
   0x100000, not 0xFFFF0 as it should.
 - Do not display the DMI version if it would be 0.0.
 - Remove senseless tests in dump (debug) code.

Other changes are simple cleanups and should be straightforward to understand.

All comments are of course welcome.

--- linux-2.2.22/arch/i386/kernel/dmi_scan.c.orig	Sun Mar 25 18:37:29 2001
+++ linux-2.2.22/arch/i386/kernel/dmi_scan.c	Wed Oct  2 14:43:22 2002
@@ -32,7 +32,6 @@
 	struct dmi_header *dm;
 	u8 *data;
 	int i=1;
-	int last = 0;		
 		
 	buf = ioremap(base, len);
 	if(buf==NULL)
@@ -42,9 +41,6 @@
 	while(i<num && (data-buf) < len)
 	{
 		dm=(struct dmi_header *)data;
-		if(dm->type < last)
-			break;
-		last = dm->type;
 		decode(dm);		
 		data+=dm->length;
 		while(*data || data[1])
@@ -57,33 +53,48 @@
 }
 
 
+inline int __init dmi_checksum(u8 *buf)
+{
+	u8 sum=0;
+	int a;
+	
+	for(a=0; a<15; a++)
+		sum+=buf[a];
+	return (sum==0);
+}
+
 int __init dmi_iterate(void (*decode)(struct dmi_header *))
 {
-	unsigned char buf[20];
-	long fp=0xE0000L;
-	fp -= 16;
+	u8 buf[15];
+	u32 fp=0xF0000;
 	
 	while( fp < 0xFFFFF)
 	{
-		fp+=16;
-		memcpy_fromio(buf, fp, 20);
-		if(memcmp(buf, "_DMI_", 5)==0)
+		memcpy_fromio(buf, fp, 15);
+		if(memcmp(buf, "_DMI_", 5)==0 && dmi_checksum(buf))
 		{
 			u16 num=buf[13]<<8|buf[12];
 			u16 len=buf[7]<<8|buf[6];
 			u32 base=buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8];
 #ifdef DUMP_DMI
-			printk(KERN_INFO "DMI %d.%d present.\n",
-				buf[14]>>4, buf[14]&0x0F);
+			/*
+			 * DMI version 0.0 means that the real version is taken from
+			 * the SMBIOS version, which we don't know at this point.
+			 */
+			if(buf[14]!=0)
+				printk(KERN_INFO "DMI %u.%u present.\n",
+					buf[14]>>4, buf[14]&0x0F);
+			else
+				printk(KERN_INFO "DMI present.\n");
 			printk(KERN_INFO "%d structures occupying %d bytes.\n",
-				buf[13]<<8|buf[12],
-				buf[7]<<8|buf[6]);
+				num, len);
 			printk(KERN_INFO "DMI table at 0x%08X.\n",
-				buf[11]<<24|buf[10]<<16|buf[9]<<8|buf[8]);
+				base);
 #endif				
 			if(dmi_table(base,len, num, decode)==0)
 				return 0;
 		}
+		fp+=16;
 	}
 	return -1;
 }
@@ -98,21 +109,17 @@
 static void __init dmi_decode(struct dmi_header *dm)
 {
 	u8 *data = (u8 *)dm;
-	char *p;
 	
 	switch(dm->type)
 	{
 		case  0:		
 #ifdef DUMP_DMI
-			p=dmi_string(dm,data[4]);
-			if(*p && *p!=' ')
-			{
-				printk("BIOS Vendor: %s\n", p);
-				printk("BIOS Version: %s\n", 
-					dmi_string(dm, data[5]));
-				printk("BIOS Release: %s\n",
-					dmi_string(dm, data[8]));
-			}
+			printk("BIOS Vendor: %s\n",
+				dmi_string(dm, data[4]));
+			printk("BIOS Version: %s\n", 
+				dmi_string(dm, data[5]));
+			printk("BIOS Release: %s\n",
+				dmi_string(dm, data[8]));
 #endif				
 			/*
 			 *  Check for clue free BIOS implementations who use
@@ -142,35 +149,22 @@
 			break;
 #ifdef DUMP_DMI
 		case 1:
-			p=dmi_string(dm,data[4]);
-
-			if(*p && *p!=' ')
-			{
-				printk("System Vendor: %s.\n",p);
-				printk("Product Name: %s.\n",
-					dmi_string(dm, data[5]));
-				printk("Version %s.\n",
-					dmi_string(dm, data[6]));
-				printk("Serial Number %s.\n",
-					dmi_string(dm, data[7]));
-			}
+			printk("System Vendor: %s\n",
+				dmi_string(dm, data[4]));
+			printk("Product Name: %s\n",
+				dmi_string(dm, data[5]));
+			printk("Version: %s\n",
+				dmi_string(dm, data[6]));
+			printk("Serial Number: %s\n",
+				dmi_string(dm, data[7]));
 			break;
 		case 2:
-			p=dmi_string(dm,data[4]);
-
-			if(*p && *p!=' ')
-			{
-				printk("Board Vendor: %s.\n",p);
-				printk("Board Name: %s.\n",
+			printk("Board Vendor: %s\n",
+				dmi_string(dm, data[4]));
+			printk("Board Name: %s\n",
 				dmi_string(dm, data[5]));
-				printk("Board Version: %s.\n",
-					dmi_string(dm, data[6]));
-			}
-			break;
-		case 3:
-			p=dmi_string(dm,data[8]);
-			if(*p && *p!=' ')
-				printk("Asset Tag: %s.\n", p);
+			printk("Board Version: %s\n",
+				dmi_string(dm, data[6]));
 			break;
 #endif			
 	}


___________________________________
Webmail Nerim, http://www.nerim.net/