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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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  <address@hidden>
> +
> +     * 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
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel
>   


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

signature.asc
Description: OpenPGP digital signature