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

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

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

Hi,

This is the reworked version of the patch.

> There are few ramifications of this patch. First of all some partitions
> which are just barely outside of the host partition will lead to
> something like "partition not found" errors in grub-probe. This message
> should be more informative (the easiest way is to issue a warning in
> grub-probe if partitions are discarded except some cases where it's
> known not to affect the functionality like 'd' "subpartitions", probably
> such a warning in grub proper would be too annoying though).

There is now a grub_dprintf where the partition is discarded.

> Then if you check partitions when iterating no need to recheck in
> adjust_range.

I simplified the check in grub_disk_adjust_range: no need to check for
the ``ancestor'' partitions, but we still check for the given partition.

>> 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?

I left this out.  Even if it introduces redundancy, it's actually nice
to see (hd1,msdos1,bsd1) in ls -l (e.g., if bsd1 is the only partition
in the bsd label).

Grégoire

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: strict-partition-nesting.diff --]
[-- Type: text/x-patch; name="strict-partition-nesting.diff", Size: 6012 bytes --]

=== added file 'ChangeLog.strict-partition-nesting'
--- ChangeLog.strict-partition-nesting	1970-01-01 00:00:00 +0000
+++ ChangeLog.strict-partition-nesting	2010-07-06 21:25:28 +0000
@@ -0,0 +1,14 @@
+2010-07-06  Grégoire Sutre  <gregoire.sutre@gmail.com>
+
+	* kern/partition.c (grub_partition_validate): New function to check
+	that a partition is physically contained in its parent.  Since offsets
+	are relative (and non-negative), this reduces to checking that the
+	partition ends before its parent.
+	(grub_partition_map_probe): Discard invalid partitions.
+	(grub_partition_iterate): Likewise.
+	* kern/disk.c (grub_disk_adjust_range): Simplify out-of-partition
+	check (partitions are assumed to be properly nested).
+	* include/grub/partition.h (grub_partition_map): Slightly more detailed
+	comments.
+	* partmap/bsdlabel.c (bsdlabel_partition_map_iterate): Discard
+	partitions that start before their parent, and add debug printfs.

=== modified file 'include/grub/partition.h'
--- include/grub/partition.h	2010-06-12 13:33:09 +0000
+++ include/grub/partition.h	2010-07-06 21:16:05 +0000
@@ -48,7 +48,7 @@ struct grub_partition
   /* The partition number.  */
   int number;
 
-  /* The start sector.  */
+  /* The start sector (relative to parent).  */
   grub_disk_addr_t start;
 
   /* The length in sector units.  */
@@ -60,7 +60,7 @@ struct grub_partition
   /* The index of this partition in the partition table.  */
   int index;
 
-  /* Parent partition map.  */
+  /* Parent partition (physically contains this partition).  */
   struct grub_partition *parent;
 
   /* The type partition map.  */

=== modified file 'kern/disk.c'
--- kern/disk.c	2010-02-07 00:48:38 +0000
+++ kern/disk.c	2010-07-06 21:22:27 +0000
@@ -361,12 +361,10 @@ grub_disk_adjust_range (grub_disk_t disk
   *sector += *offset >> GRUB_DISK_SECTOR_BITS;
   *offset &= GRUB_DISK_SECTOR_SIZE - 1;
 
-  for (part = disk->partition; part; part = part->parent)
+  part = disk->partition;
+  if (part)
     {
-      grub_disk_addr_t start;
       grub_uint64_t len;
-
-      start = part->start;
       len = part->len;
 
       if (*sector >= len
@@ -374,7 +372,7 @@ grub_disk_adjust_range (grub_disk_t disk
 			      >> GRUB_DISK_SECTOR_BITS))
 	return grub_error (GRUB_ERR_OUT_OF_RANGE, "out of partition");
 
-      *sector += start;
+      *sector += grub_partition_get_start (part);
     }
 
   if (disk->total_sectors <= *sector

=== modified file 'kern/partition.c'
--- kern/partition.c	2010-02-06 20:00:53 +0000
+++ kern/partition.c	2010-07-06 21:20:04 +0000
@@ -23,6 +23,23 @@
 
 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)
@@ -31,20 +48,26 @@ grub_partition_map_probe (const grub_par
 
   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)
-	{
-	  p = (grub_partition_t) grub_malloc (sizeof (*p));
-	  if (! p)
-	    return 1;
+      if (partnum != partition->number)
+	return 0;
 
-	  grub_memcpy (p, partition, sizeof (*p));
-	  return 1;
+      if (!(grub_partition_validate (dsk, partition)))
+	{
+	  grub_dprintf ("partition", "Invalid (sub-)partition %s%d (%s).\n",
+			partition->partmap->name, partition->number + 1,
+			"out of range");
+	  return 0;
 	}
 
-      return 0;
+      p = (grub_partition_t) grub_malloc (sizeof (*p));
+      if (! p)
+	return 1;
+
+      grub_memcpy (p, partition, sizeof (*p));
+      return 1;
     }
 
   partmap->iterate (disk, find_func);
@@ -138,6 +161,15 @@ grub_partition_iterate (struct grub_disk
 		    const grub_partition_t partition)
     {
       struct grub_partition p = *partition;
+
+      if (!(grub_partition_validate (dsk, partition)))
+	{
+	  grub_dprintf ("partition", "Invalid (sub-)partition %s%d (%s).\n",
+			partition->partmap->name, partition->number + 1,
+			"out of range");
+	  return 0;
+	}
+
       p.parent = dsk->partition;
       dsk->partition = 0;
       if (hook (dsk, &p))

=== modified file 'partmap/bsdlabel.c'
--- partmap/bsdlabel.c	2010-03-26 14:44:13 +0000
+++ partmap/bsdlabel.c	2010-07-06 21:15:19 +0000
@@ -54,7 +54,7 @@ bsdlabel_partition_map_iterate (grub_dis
 
   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;
 
@@ -64,15 +64,29 @@ bsdlabel_partition_map_iterate (grub_dis
       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)
+	{
+	  grub_dprintf ("partition",
+			"partition %d: invalid start (found 0x%llx, wanted >= 0x%llx)\n",
+			p.number,
+			(unsigned long long) p.start,
+			(unsigned long long) 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;