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

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

[Grégoire Sutre]

[-- 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",

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

[Seth Goldberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2093 bytes --]


  This patch will also fix (i.e. Solaris), which embeds a copy of the MBR's 
partition table in the PBR.

  --S

Quoting Grégoire Sutre, who wrote the following on Fri, 28 May 2010:

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

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

Grégoire Sutre wrote:
> 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.
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).
Then if you check partitions when iterating no need to recheck in
adjust_range.
>
> 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?
No. SUN partitions comonly start at offset 0.
>
>
> Grégoire
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

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

[Grégoire Sutre]

On 05/31/2010 08:35 PM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

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

It's not ideal, but IMHO it's better than getting "unknown filesystem"
in grub-probe -t fs while the partition is detected fine by
-t drive.

> 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).

A grub_dprintf when the partition is discarded (in the proposed patch)
would only print the message for relevant partitions.

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

Agreed.

>> 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?
> No. SUN partitions comonly start at offset 0.

I don't understand what you mean: here bsd1 also starts at (relative)
offset 0, and the above example actually assumed that.

By the way, when several partition identifiers denote the same
partition, the MBI boot_device field can have different values for the
same physical partition.  This means more work (or assumptions) on the
kernel side to identify the root partition.  In the above example,
(hd1,msdos1,bsd1) would be more explicit to the NetBSD kernel than
(hd1,msdos1).

Grégoire

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

[Grégoire Sutre]

[-- 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;

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 07/06/2010 11:55 PM, Grégoire Sutre wrote:
> 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.
>
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
>> 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.
>
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.
>>> 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).
>
OK
> === 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;
>   
[grub_disk_get_len does exactly that]
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.
We must always exercice best effort strategy. If something can bee
booted, boot it.
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)
> +
> +  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,
> +	
Ditto
> 		"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);
>   
grub_util_warn is needed here too.
> +	  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;
>
>   
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

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

[Grégoire Sutre]

[-- 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;

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 07/09/2010 12:53 PM, Grégoire Sutre wrote:
> 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.
In practice they only confuse search command.
>   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.
I mean that the files we need are below the limit but some parts of
partition are above the limit. It will work the current boot but will
break in the future.
>
> Grégoire
>
> strict-partition-nesting_v3.diff
>
>
> === 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;
>
>   
Go ahead for trunk.
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]

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

[Grégoire Sutre]

On 07/13/2010 11:53 AM, Vladimir 'φ-coder/phcoder' Serbinenko wrote:

> I mean that the files we need are below the limit but some parts of
> partition are above the limit. It will work the current boot but will
> break in the future.

I see.


> Go ahead for trunk.

Thanks, I committed the patch.

Grégoire