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

[Christian Franke]

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

--- 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);