[Patch] Discard incorrect nested partitions (fixes #29956)

["Grégoire Sutre" <gregoire.sutre@gmail.com>]

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

Hi,

Regarding the nested partition code, there is an implicit assumption
that each partition should be contained in its parent, i.e. its sectors
  should also be sectors of its parent.

This ``physical nesting'' is checked in grub_disk_read, but it would
be better to check it before that.

The attached patch discards partitions that are invalid w.r.t. physical
nesting.  This solves, in particular, a problem related to NetBSD (and
OpenBSD) disklabels.

With this patch, ``external'' partitions in a disklabel simply do not
show up as BSD partitions.  For instance (see bug #29956 for an image):

MBR Partition table:
0: NetBSD    start 32, size 1000
1: DOS       start 1040, size 1000

NetBSD Disklabel (stored in MBR partition 0)
5 partitions:
#        size    offset     fstype [fsize bsize cpg/sgs]
  a:      1000        32     4.2BSD
  c:      1000        32     unused
  d:      2048         0     unused
  e:      1000      1040      MSDOS


The e: partition is external: it is not contained in MBR NetBSD
partition.

Without the patch, we get:

$ grub-probe -m /dev/null -t drive -d /dev/rvnd0e
(/dev/rvnd0d,1,5)   # this is (/dev/rvnd0d,msdos1,bsd5)
$ grub-probe -m /dev/null -t fs -d /dev/rvnd0e
grub-probe: error: unknown filesystem.

With the patch, we get:

niagara# grub-probe -m /dev/null -t drive -d /dev/rvnd0e
(/dev/rvnd0d,2)   # this is (/dev/rvnd0d,msdos2)
niagara# grub-probe -m /dev/null -t fs -d /dev/rvnd0e
fat


The patch still accepts sub-partitions that start at the same
(absolute) offset as the parent.  For instance, in the above example,
ls -l in grub gives both (hd1,msdos1) and (hd1,msdos1,bsd1).  Should
we discard (hd0,msdos1,bsd1), i.e. require that sub-partitions start
at a strictly positive relative offset?

Grégoire


[-- Attachment #2: nesting.diff --]
[-- Type: text/x-patch, Size: 3732 bytes --]

--- grub_trunk/kern/partition.c	2010-05-28 01:50:37.000000000 +0200
+++ grub_trunk_new/kern/partition.c	2010-05-28 01:53:13.000000000 +0200
@@ -16,32 +16,50 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <grub/misc.h>
 #include <grub/mm.h>
 #include <grub/partition.h>
 #include <grub/disk.h>
 
 grub_partition_map_t grub_partition_map_list;
 
+/*
+ * Checks that a partition is contained in its parent.
+ */
+static int
+grub_partition_validate (const grub_disk_t disk,
+			 const grub_partition_t partition)
+{
+  grub_disk_addr_t parent_len;
+
+  if (disk->partition)
+    parent_len = grub_partition_get_len (disk->partition);
+  else
+    parent_len = disk->total_sectors;
+
+  return (partition->start + partition->len <= parent_len);
+}
+
 static grub_partition_t
 grub_partition_map_probe (const grub_partition_map_t partmap,
 			  grub_disk_t disk, int partnum)
 {
   grub_partition_t p = 0;
 
   auto int find_func (grub_disk_t d, const grub_partition_t partition);
 
-  int find_func (grub_disk_t d __attribute__ ((unused)),
+  int find_func (grub_disk_t dsk,
 		 const grub_partition_t partition)
     {
-      if (partnum == partition->number)
+      if (partnum == partition->number &&
+	  grub_partition_validate (dsk, partition))
 	{
 	  p = (grub_partition_t) grub_malloc (sizeof (*p));
 	  if (! p)
 	    return 1;
 
 	  grub_memcpy (p, partition, sizeof (*p));
 	  return 1;
 	}
 
       return 0;
@@ -131,20 +149,24 @@ grub_partition_iterate (struct grub_disk
 				     const grub_partition_t partition))
 {
   int ret = 0;
 
   auto int part_iterate (grub_disk_t dsk, const grub_partition_t p);
 
   int part_iterate (grub_disk_t dsk,
 		    const grub_partition_t partition)
     {
       struct grub_partition p = *partition;
+
+      if (!(grub_partition_validate (dsk, partition)))
+	return 0;
+
       p.parent = dsk->partition;
       dsk->partition = 0;
       if (hook (dsk, &p))
 	{
 	  ret = 1;
 	  return 1;
 	}
       if (p.start != 0)
 	{
 	  const struct grub_partition_map *partmap;
--- grub_trunk/partmap/bsdlabel.c	2010-05-28 01:50:39.000000000 +0200
+++ grub_trunk_new/partmap/bsdlabel.c	2010-05-28 02:10:26.000000000 +0200
@@ -47,39 +47,46 @@ bsdlabel_partition_map_iterate (grub_dis
 
   /* Check if it is valid.  */
   if (label.magic != grub_cpu_to_le32 (GRUB_PC_PARTITION_BSD_LABEL_MAGIC))
     return grub_error (GRUB_ERR_BAD_PART_TABLE, "no signature");
 
   pos = sizeof (label) + GRUB_PC_PARTITION_BSD_LABEL_SECTOR
     * GRUB_DISK_SECTOR_SIZE;
 
   for (p.number = 0;
        p.number < grub_cpu_to_le16 (label.num_partitions);
-       p.number++)
+       p.number++, pos += sizeof (struct grub_partition_bsd_entry))
     {
       struct grub_partition_bsd_entry be;
 
       p.offset = pos / GRUB_DISK_SECTOR_SIZE;
       p.index = pos % GRUB_DISK_SECTOR_SIZE;
 
       if (grub_disk_read (disk, p.offset, p.index, sizeof (be),  &be))
 	return grub_errno;
 
-      p.start = grub_le_to_cpu32 (be.offset) - delta;
+      p.start = grub_le_to_cpu32 (be.offset);
+      if (p.start < delta)
+	continue;
+      p.start -= delta;
       p.len = grub_le_to_cpu32 (be.size);
       p.partmap = &grub_bsdlabel_partition_map;
 
+      grub_dprintf ("partition",
+		    "partition %d: type 0x%x, start 0x%llx, len 0x%llx\n",
+		    p.number, be.fs_type,
+		    (unsigned long long) p.start,
+		    (unsigned long long) p.len);
+
       if (be.fs_type != GRUB_PC_PARTITION_BSD_TYPE_UNUSED)
 	if (hook (disk, &p))
 	  return grub_errno;
-
-      pos += sizeof (struct grub_partition_bsd_entry);
     }
 
   return GRUB_ERR_NONE;
 }
 
 \f
 /* Partition map type.  */
 static struct grub_partition_map grub_bsdlabel_partition_map =
   {
     .name = "bsd",