From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1OYcAt-00044o-2w for mharc-grub-devel@gnu.org; Tue, 13 Jul 2010 05:53:15 -0400 Received: from [140.186.70.92] (port=33472 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OYcAn-00041v-IO for grub-devel@gnu.org; Tue, 13 Jul 2010 05:53:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OYcAm-0006Xf-4H for grub-devel@gnu.org; Tue, 13 Jul 2010 05:53:09 -0400 Received: from mail-bw0-f41.google.com ([209.85.214.41]:49992) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OYcAl-0006XX-Q2 for grub-devel@gnu.org; Tue, 13 Jul 2010 05:53:08 -0400 Received: by bwz9 with SMTP id 9so3790250bwz.0 for ; Tue, 13 Jul 2010 02:53:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from :user-agent:mime-version:to:subject:references:in-reply-to :x-enigmail-version:content-type; bh=j38iqBrktQ3nC35ptpdgluDXqW4b/1I5oSH9NsTPE8A=; b=IRAEELnqIHNosXglhVLkYw4OBj6HIKpJ+tMopvDXCTezxJDV30K2+Wg75jBA5a5sn6 6HJ6p0aV6ZUSGUwcY21jdgnpBHVbK991fzKRAfvdZ5HadCUdySJm4mjcvPXvgB2F8+n5 WUV/u2NI/6AaVVh0FrtYKcQeCe6iRvKfiV0fk= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:x-enigmail-version:content-type; b=X5/lEjOZwQSPaOC6NwR0xmIOhEit1oUn3rGJ8naSn8hH5pv8iQN2lwfRt6a3ogPFeZ YHQGrqqFqXzTkLxT3X240oAHu25poPr8bBA3/8TcnZVOk+g3aCKMYjTQ/Ws/od6KpvMx jMD5WgRu3FCmOedKB/ba74ECWDPlUdo5Anrxw= Received: by 10.204.10.134 with SMTP id p6mr11796451bkp.206.1279014784765; Tue, 13 Jul 2010 02:53:04 -0700 (PDT) Received: from debian.bg45.phnet (vpn-global-158-dhcp.ethz.ch [129.132.211.158]) by mx.google.com with ESMTPS id s17sm22953837bkx.18.2010.07.13.02.53.01 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 13 Jul 2010 02:53:02 -0700 (PDT) Message-ID: <4C3C377C.70602@gmail.com> Date: Tue, 13 Jul 2010 11:53:00 +0200 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.10) Gecko/20100620 Icedove/3.0.5 MIME-Version: 1.0 To: grub-devel@gnu.org References: <4BFF1337.3020100@gmail.com> <4C040166.3030502@gmail.com> <4C33A65A.7090005@gmail.com> <4C351BB9.9020802@gmail.com> <4C36FFAA.5000100@gmail.com> In-Reply-To: <4C36FFAA.5000100@gmail.com> X-Enigmail-Version: 1.0.1 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="------------enigBABC85C631D5A6442102584C" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) Subject: Re: [Patch] Discard incorrect nested partitions (fixes #29956) X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Jul 2010 09:53:11 -0000 This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enigBABC85C631D5A6442102584C Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 07/09/2010 12:53 PM, Gr=C3=A9goire Sutre wrote: > On 07/08/2010 02:28, Vladimir '=CF=86-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 avoi= d > 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 fo= r >>> 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 th= is >> 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 dis= k >> 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 outsid= e >> 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 t= o >> 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=C3=A9goire > > strict-partition-nesting_v3.diff > > > =3D=3D=3D 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=C3=A9goire Sutre > + > + * kern/partition.c (grub_partition_check_containment): New function t= o > + check that a partition is physically contained in a parent. Since > + offsets are relative (and non-negative), this reduces to checking tha= t > + 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 detail= ed > + comments. > + * partmap/bsdlabel.c (bsdlabel_partition_map_iterate): Discard > + partitions that start before their parent, and add debug printfs. > > =3D=3D=3D 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; > =20 > - /* The start sector. */ > + /* The start sector (relative to parent). */ > grub_disk_addr_t start; > =20 > /* The length in sector units. */ > @@ -60,7 +60,7 @@ struct grub_partition > /* The index of this partition in the partition table. */ > int index; > =20 > - /* Parent partition map. */ > + /* Parent partition (physically contains this partition). */ > struct grub_partition *parent; > =20 > /* The type partition map. */ > > =3D=3D=3D 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 @@ > =20 > grub_partition_map_t grub_partition_map_list; > =20 > +/* > + * Checks that disk->partition contains part. This function assumes t= hat 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 =3D=3D NULL) > + return 1; > + > + if (part->start + part->len > disk->partition->len) > + { > + char *partname; > + > + partname =3D grub_partition_get_name (disk->partition); > + grub_dprintf ("partition", "sub-partition %s%d of (%s,%s) ends a= fter 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 > =20 > auto int find_func (grub_disk_t d, const grub_partition_t partition)= ; > =20 > - int find_func (grub_disk_t d __attribute__ ((unused)), > + int find_func (grub_disk_t dsk, > const grub_partition_t partition) > { > - if (partnum =3D=3D partition->number) > - { > - p =3D (grub_partition_t) grub_malloc (sizeof (*p)); > - if (! p) > - return 1; > + if (partnum !=3D partition->number) > + return 0; > =20 > - grub_memcpy (p, partition, sizeof (*p)); > - return 1; > - } > + if (!(grub_partition_check_containment (dsk, partition))) > + return 0; > =20 > - return 0; > + p =3D (grub_partition_t) grub_malloc (sizeof (*p)); > + if (! p) > + return 1; > + > + grub_memcpy (p, partition, sizeof (*p)); > + return 1; > } > =20 > partmap->iterate (disk, find_func); > @@ -138,6 +170,10 @@ grub_partition_iterate (struct grub_disk > const grub_partition_t partition) > { > struct grub_partition p =3D *partition; > + > + if (!(grub_partition_check_containment (dsk, partition))) > + return 0; > + > p.parent =3D dsk->partition; > dsk->partition =3D 0; > if (hook (dsk, &p)) > > =3D=3D=3D 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 > =20 > for (p.number =3D 0; > p.number < grub_cpu_to_le16 (label.num_partitions); > - p.number++) > + p.number++, pos +=3D sizeof (struct grub_partition_bsd_entry)) > { > struct grub_partition_bsd_entry be; > =20 > @@ -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; > =20 > - p.start =3D grub_le_to_cpu32 (be.offset) - delta; > + p.start =3D grub_le_to_cpu32 (be.offset); > p.len =3D grub_le_to_cpu32 (be.size); > p.partmap =3D &grub_bsdlabel_partition_map; > =20 > - if (be.fs_type !=3D 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 =3D=3D 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 >=3D 0x%llx)\n",= > + p.number, > + (unsigned long long) p.start, > + (unsigned long long) delta); > +#ifdef GRUB_UTIL > + /* disk->partition !=3D NULL as 0 < delta */ > + partname =3D 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; > + } > =20 > - pos +=3D sizeof (struct grub_partition_bsd_entry); > + p.start -=3D delta; > + > + if (hook (disk, &p)) > + return grub_errno; > } > =20 > return GRUB_ERR_NONE; > > =20 Go ahead for trunk. > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel > =20 --=20 Regards Vladimir '=CF=86-coder/phcoder' Serbinenko --------------enigBABC85C631D5A6442102584C Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iF4EAREKAAYFAkw8N3wACgkQNak7dOguQglMEAEAp1Qy/6aSfBGJOuFcy+lds82N oKDe+gA0NERr+BAr5xMA/3y5n+3oAhx0ZknFoRy/sbatidRLEHdId0LkrlsTWSAP =jQzN -----END PGP SIGNATURE----- --------------enigBABC85C631D5A6442102584C--