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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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


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

signature.asc
Description: OpenPGP digital signature