Re: [PATCH] Fix eisa_mmap evaluation, add memory existence check

[Christian Franke <Christian.Franke@t-online.de>]

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

Robert Millan wrote:
> On Tue, Oct 23, 2007 at 09:06:16PM +0200, Christian Franke wrote:
>   
>> +/* Check memory address */
>> +static int
>> +addr_is_valid (grub_addr_t addr)
>> +{
>> +  volatile unsigned char * p = (volatile unsigned char *)addr;
>> +  unsigned char x, y;
>> +  x = *p;
>> +  *p = x ^ 0xcf;
>> +  y = *p;
>> +  *p = x;
>> +  return y == (x ^ 0xcf);
>> +}
>>     
>
> 0xff would be better IMO.
>
>   

Done.

>> +  if (!(addr + size > addr && addr_is_valid (addr) && addr_is_valid (addr+size-1)))
>> +    grub_fatal ("invalid memory region %p - %p", (char*)addr, (char*)addr+size-1);
>>     
>
> Should `addr + size > addr' be optimized out as `size > 0' ?  (or if we need it
> this way to check for overflows, should we prevent gcc from optimizing it?)
>
>   

Good point.

It worked (I usually test all corner cases before patch release). And a 
review of .S file shows that gcc (3.4.4) does not optimize here. I'm not 
sure whether that would be allowed.

But you are right - such check should not depend on specific overflow 
behaviour. I've changed this.

New patch attached, old changelog still valid.

Christian


[-- Attachment #2: grub2-init-eisa_mmap-2.patch --]
[-- Type: text/x-patch, Size: 1489 bytes --]

--- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
+++ grub2/kern/i386/pc/init.c	2007-11-19 22:11:19.796875000 +0100
@@ -83,6 +83,19 @@ make_install_device (void)
   return grub_prefix;
 }
 
+/* Check memory address.  */
+static int
+addr_is_valid (grub_addr_t addr)
+{
+  volatile unsigned char * p = (volatile unsigned char *)addr;
+  unsigned char x = *p;
+  unsigned char y = ~x;
+  *p = y;
+  unsigned char t = *p;
+  *p = x;
+  return y == t;
+}
+
 /* Add a memory region.  */
 static void
 add_mem_region (grub_addr_t addr, grub_size_t size)
@@ -91,6 +104,10 @@ add_mem_region (grub_addr_t addr, grub_s
     /* Ignore.  */
     return;
 
+  if (!(0 < size && size - 1 <= ~(grub_addr_t)0 - addr &&
+        addr_is_valid (addr) && addr_is_valid (addr + size - 1)))
+    grub_fatal ("invalid memory region %p - %p", (char*)addr, (char*)addr + size - 1);
+
   mem_regions[num_regions].addr = addr;
   mem_regions[num_regions].size = size;
   num_regions++;
@@ -199,13 +216,8 @@ grub_machine_init (void)
 
       if (eisa_mmap)
 	{
-	  if ((eisa_mmap & 0xFFFF) == 0x3C00)
-	    add_mem_region (0x100000, (eisa_mmap << 16) + 0x100000 * 15);
-	  else
-	    {
-	      add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
-	      add_mem_region (0x1000000, eisa_mmap << 16);
-	    }
+	  add_mem_region (0x100000, (eisa_mmap & 0xFFFF) << 10);
+	  add_mem_region (0x1000000, eisa_mmap & ~0xFFFF);
 	}
       else
 	add_mem_region (0x100000, grub_get_memsize (1) << 10);