Re: [PATCH]: grub: Fix ofdisk disk cache corruption.

[Pavel Roskin <proski@gnu.org>]

On Sat, 2009-04-11 at 01:08 -0700, David Miller wrote:
> The ieee1275 ofdisk driver doesn't use a unique value for
> disk->id so it's really easy to get disk corruption.  I was
> able to see such corruption by simply booting grub from one
> disk and booting a Linux kernel from another, both of which
> were on the same disk controller.

I hope you mean disk cache corruption, as in the subject, not disk
corruption.  GRUB only writes to disks to save environment variables,
and it's done very carefully.  

> +#define OFDISK_HASH_SZ	8
> +static struct ofdisk_hash_ent *ofdisk_hash[OFDISK_HASH_SZ];
> +
> +static int
> +ofdisk_hash_fn (const char *devpath)
> +{
> +  int hash = 0;
> +  while (*devpath)
> +    hash ^= *devpath++;
> +  return (hash & (OFDISK_HASH_SZ - 1));
> +}

That's a 3 bit hash.  The risk of collisions is very high.  I would
understand if you had 8 entries for the hash values, but the hash values
themselves should be reasonably unique.

> +static struct ofdisk_hash_ent *
> +ofdisk_hash_add (char *devpath)
> +{
> +  struct ofdisk_hash_ent **head = &ofdisk_hash[ofdisk_hash_fn(devpath)];
> +  struct ofdisk_hash_ent *p = grub_malloc(sizeof (*p));
> +
> +  if (p)
> +    {
> +      p->devpath = devpath;

If you can save the device names, then there is no point in using
hashes.  You can use (long)devpath.

> +  if (!op)
> +    op = ofdisk_hash_add (devpath);
>  
> -  grub_ieee1275_open (devpath, &dev_ihandle);
> +  grub_free (devpath);

But if you free the device names, then they are bad IDs.  The
probability of the same memory being reused for another name is high.

Perhaps I misunderstand something.

-- 
Regards,
Pavel Roskin