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

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

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

On 07/08/2010 02:28, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

Attached is the new version of the patch.

> As I already told you in real dprintf isn't seen by user. One need to
> grub_dprintf (....);
> #ifdef GRUB_UTIL
> grub_util_warn (...);
> #endif

Ok.  For partition.c, this is now done in the checking function to avoid
code duplication (and it makes the code easier to read).

For bsdlabel.c, I re-ordered the code so that no warning is shown for
partitions of type unused (such as the raw partition).

>> 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.
>>
> You're right. This means we can't screw this test to save space. In this
> case it's better to do the complete check for early bug catch.

I removed the simplification from the patch.

> We shouldn't check for partitions being outside of disk since BIOS disk
> size limitations are common. Consider following situation:
> (hd0,msdos1,bsd1)           /boot
> BIOS LIMIT
> (hd0,msdos1,bsd2)          /
> This system is perfectly capable of booting but with your patch it won't.

Right.  The patch now only checks sub-partitions (no check is done on
top-level partitions).

 > We must always exercice best effort strategy. If something can bee
 > booted, boot it.

Here, we discard improperly nested partitions, even though they could
be accessed.  So one may argue that this breaks the best effort
strategy.  However, such improperly nested partitions can in general be
accessed by a properly nested identifier, so I guess it's fine.

> We should warn if a used final-nestedness partition is partialy outside
> the limit. Simple message usually scrolls way too fast and so usually
> ignored (if someone sees boot process at all). Perhaps we need a way to
> pass such warnings to kernel which then can take appropriate action
> (e.g. notify sysadmin)

I'm not sure where we should check that.  If one attempts to load a
kernel or module that is beyond the disk limit, grub_disk_read will
fail anyway.

Grégoire


[-- Attachment #2: strict-partition-nesting_v3.diff --]
[-- Type: text/plain, Size: 5781 bytes --]

=== added file 'ChangeLog.strict-partition-nesting'
--- ChangeLog.strict-partition-nesting	1970-01-01 00:00:00 +0000
+++ ChangeLog.strict-partition-nesting	2010-07-09 00:26:38 +0000
@@ -0,0 +1,12 @@
+2010-07-06  Grégoire Sutre  <gregoire.sutre@gmail.com>
+
+	* kern/partition.c (grub_partition_check_containment): New function to
+	check that a partition is physically contained in a parent.  Since
+	offsets are relative (and non-negative), this reduces to checking that
+	the partition ends before its parent.
+	(grub_partition_map_probe): Discard out-of-range sub-partitions.
+	(grub_partition_iterate): Likewise.
+	* 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-09 00:26:38 +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/partition.c'
--- kern/partition.c	2010-02-06 20:00:53 +0000
+++ kern/partition.c	2010-07-09 00:26:38 +0000
@@ -23,6 +23,37 @@
 
 grub_partition_map_t grub_partition_map_list;
 
+/*
+ * Checks that disk->partition contains part.  This function assumes that the
+ * start of part is relative to the start of disk->partition.  Returns 1 if
+ * disk->partition is null.
+ */
+static int
+grub_partition_check_containment (const grub_disk_t disk,
+				  const grub_partition_t part)
+{
+  if (disk->partition == NULL)
+    return 1;
+
+  if (part->start + part->len > disk->partition->len)
+    {
+      char *partname;
+
+      partname = grub_partition_get_name (disk->partition);
+      grub_dprintf ("partition", "sub-partition %s%d of (%s,%s) ends after parent.\n",
+		    part->partmap->name, part->number + 1, disk->name, partname);
+#ifdef GRUB_UTIL
+      grub_util_warn ("Discarding improperly nested partition (%s,%s,%s%d)",
+		      disk->name, partname, part->partmap->name, part->number + 1);
+#endif
+      grub_free (partname);
+
+      return 0;
+    }
+
+  return 1;
+}
+
 static grub_partition_t
 grub_partition_map_probe (const grub_partition_map_t partmap,
 			  grub_disk_t disk, int partnum)
@@ -31,20 +62,21 @@ 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_check_containment (dsk, partition)))
+	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 +170,10 @@ grub_partition_iterate (struct grub_disk
 		    const grub_partition_t partition)
     {
       struct grub_partition p = *partition;
+
+      if (!(grub_partition_check_containment (dsk, partition)))
+	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-09 00:26:38 +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,43 @@ 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);
       p.len = grub_le_to_cpu32 (be.size);
       p.partmap = &grub_bsdlabel_partition_map;
 
-      if (be.fs_type != GRUB_PC_PARTITION_BSD_TYPE_UNUSED)
-	if (hook (disk, &p))
-	  return grub_errno;
+      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)
+	continue;
+
+      if (p.start < delta)
+	{
+#ifdef GRUB_UTIL
+	  char *partname;
+#endif
+	  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);
+#ifdef GRUB_UTIL
+	  /* disk->partition != NULL as 0 < delta */
+	  partname = grub_partition_get_name (disk->partition);
+	  grub_util_warn ("Discarding improperly nested partition (%s,%s,%s%d)",
+			  disk->name, partname, p.partmap->name, p.number + 1);
+	  grub_free (partname);
+#endif
+	  continue;
+	}
 
-      pos += sizeof (struct grub_partition_bsd_entry);
+      p.start -= delta;
+
+      if (hook (disk, &p))
+	return grub_errno;
     }
 
   return GRUB_ERR_NONE;