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

[Marco Gerards <mgerards@xs4all.nl>]

Christian Franke <Christian.Franke@t-online.de> writes:

> This patch fixes the broken evaluation of the E801 EISA memory
> map. The shift was too much, the high word is already shifted :-) The
> bug was hidden until the E820 memory map evaluation was broken due to
> the struct packing issue fixed in my last patch.
>
> The extra handling of "0x3C00" case is IMO not necessary. Regions are
> merged a few lines later.
>
> During testing, I added a primitive memory to detect such problems
> early. It was difficult to find why grub crashes during module load.

Too be honest, I do not know this code that well.  Still, I will try
to comment on it.  Although most comments will be on style, and on the
actual code itself.

> Christian
>
> 2007-10-23  Christian Franke  <franke@computer.org>
>
> 	* kern/i386/pc/init.c (addr_is_valid): New function.
> 	(add_mem_region): Add memory existence check.
> 	(grub_machine_init): Fix evaluation of eisa_mmap.
>
>
>
> --- grub2.orig/kern/i386/pc/init.c	2007-10-22 22:22:51.359375000 +0200
> +++ grub2/kern/i386/pc/init.c	2007-10-22 22:25:44.546875000 +0200
> @@ -83,6 +83,19 @@ make_install_device (void)
>    return grub_prefix;
>  }
>  
> +/* Check memory address */

Please have a look at the GNU Coding Standards.

For comments, please use proper interpunction, so end with a `.'.
After the `.', please add two spaces before ending the comment or
before starting a new sentence.

> +static int
> +addr_is_valid (grub_addr_t addr)
> +{
> +  volatile unsigned char * p = (volatile unsigned char *)addr;

Why volatile?  I have the feeling it is not needed.

> +  unsigned char x, y;
> +  x = *p;
> +  *p = x ^ 0xcf;
> +  y = *p;
> +  *p = x;
> +  return y == (x ^ 0xcf);
> +}

Can you add some comments to this function?  It is not obvious when
and why an address is/isn't valid.

>  /* Add a memory region.  */
>  static void
>  add_mem_region (grub_addr_t addr, grub_size_t size)
> @@ -91,6 +104,9 @@ add_mem_region (grub_addr_t addr, grub_s
>      /* Ignore.  */
>      return;
>  
> +  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);

Please use more spaces:

(char *) addr + size - 1

So a space around binary operators.

>    mem_regions[num_regions].addr = addr;
>    mem_regions[num_regions].size = size;
>    num_regions++;
> @@ -199,13 +215,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);
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel