Re: rfc: test whether a device has a partition table

Re: rfc: test whether a device has a partition table

[Uwe Bonnes]

Hello,

around last september there was a discussion about the linux kernel
recognizing "supperfloppys" as disks with bogus partition tables.
Linux Torvalds wrote at one point in the discussion:
>On Thu, 25 Sep 2003, Andries Brouwer wrote:
>> 
>> My post implicitly suggested the minimal thing to do.
>> It will not be enough - heuristics are never enough -
>> but it probably helps in most cases.
>
>I don't mind the 0x00/0x80 "boot flag" checks - those look fairly 
> obvious and look reasonably safe to add to the partitioning code.
>
>There are other checks that can be done - verifying that the start/end
>sector values are at all sensible. We do _some_ of that, but only for
>partitions 3 and 4, for example. We could do more - like checking the
>actual sector numbers (but I think some formatters leave them as zero).
>
>Which actually makes me really nervous - it implies that we've probably 
>seen partitions 1&2 contain garbage there, and the problem is that if 
>you'r etoo careful in checking, you will make a system unusable.
>
>This is why it is so much nicer to be overly permissive ratehr than 
>being a stickler for having all the values right.
>
>And your random byte checks for power-of-2 make no sense. What are they
>based on?

The discussion seemed to fade out with no visible result, and for example my
USB stick "ID 0d7d:1420 Apacer" with a floppy as second partition gets
recognized as:
SCSI device sdc: 2880 512-byte hdwr sectors (1 MB)
sdc: Write Protect is off
 sdc: sdc1 sdc2 sdc3 sdc4

Find appended a patch that does the 0x00/0x80 "boot flag" checks. Please
discuss and consider for inclusion into the kernel.

Thanks

PS: CC me for faster reaction, as I only follow the mailing list via the
MARC mailing list archive.
-- 
Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------
--- linux-2.6.6/fs/partitions/msdos-sav.c	2004-05-10 04:32:52.000000000 +0200
+++ linux-2.6.6/fs/partitions/msdos.c	2004-05-22 12:54:45.000000000 +0200
@@ -32,6 +32,7 @@
  */
 #include <asm/unaligned.h>
 
+#define BOOT_IND(p)	(get_unaligned(&p->boot_ind))
 #define SYS_IND(p)	(get_unaligned(&p->sys_ind))
 #define NR_SECTS(p)	({ __typeof__(p->nr_sects) __a =	\
 				get_unaligned(&p->nr_sects);	\
@@ -377,6 +378,7 @@
 int msdos_partition(struct parsed_partitions *state, struct block_device *bdev)
 {
 	int sector_size = bdev_hardsect_size(bdev) / 512;
+	int nr_bootable = 0;
 	Sector sect;
 	unsigned char *data;
 	struct partition *p;
@@ -389,6 +391,22 @@
 		put_dev_sector(sect);
 		return 0;
 	}
+
+	/* 
+	   Some consistancy check for a valid partition table
+	   Boot indicator must either be 0x80 or 0x0 on all primary partitions
+	   Only one partition may be marked bootable (0x80)
+	*/
+ 	p = (struct partition *) (data + 0x1be);
+	for (slot = 1 ; slot <= 4 ; slot++, p++) {
+	  if ( (BOOT_IND(p) != 0x80) && (BOOT_IND(p) != 0x0))
+	    return 0;
+	  if (BOOT_IND(p) == 0x80) 
+	    nr_bootable++;
+	}
+	if (nr_bootable >1) 
+	  return 0;
+
 	p = (struct partition *) (data + 0x1be);
 #ifdef CONFIG_EFI_PARTITION
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {

Re: rfc: test whether a device has a partition table

[John Bradford]

Quote from Uwe Bonnes <bon@elektron.ikp.physik.tu-darmstadt.de>:
> Hello,
> 
> around last september there was a discussion about the linux kernel
> recognizing "supperfloppys" as disks with bogus partition tables.
> Linux Torvalds wrote at one point in the discussion:
> >On Thu, 25 Sep 2003, Andries Brouwer wrote:
> >> 
> >> My post implicitly suggested the minimal thing to do.
> >> It will not be enough - heuristics are never enough -
> >> but it probably helps in most cases.
> >
> >I don't mind the 0x00/0x80 "boot flag" checks - those look fairly 
> > obvious and look reasonably safe to add to the partitioning code.
> >
> >There are other checks that can be done - verifying that the start/end
> >sector values are at all sensible. We do _some_ of that, but only for
> >partitions 3 and 4, for example. We could do more - like checking the
> >actual sector numbers (but I think some formatters leave them as zero).
> >
> >Which actually makes me really nervous - it implies that we've probably 
> >seen partitions 1&2 contain garbage there, and the problem is that if 
> >you'r etoo careful in checking, you will make a system unusable.
> >
> >This is why it is so much nicer to be overly permissive ratehr than 
> >being a stickler for having all the values right.
> >
> >And your random byte checks for power-of-2 make no sense. What are they
> >based on?
> 
> The discussion seemed to fade out with no visible result, and for example my

I seem to remember the conclusion being Linus saying something along the
lines of prefering the situation where you have bogus partitions detected
rather than genuine partitions not detected.

John.

Re: rfc: test whether a device has a partition table

[Andries Brouwer]

On Sat, May 22, 2004 at 01:18:34PM +0200, Uwe Bonnes wrote:

> around last september there was a discussion about the linux kernel
> recognizing "supperfloppys" as disks with bogus partition tables.

Yes - already had forgotten about that - thanks for reviving

> Linux Torvalds wrote at one point in the discussion:

> >I don't mind the 0x00/0x80 "boot flag" checks - those look fairly 
> > obvious and look reasonably safe to add to the partitioning code.
> 
> The discussion seemed to fade out with no visible result, and for example my
> USB stick "ID 0d7d:1420 Apacer" with a floppy as second partition gets
> recognized as:
> SCSI device sdc: 2880 512-byte hdwr sectors (1 MB)
> sdc: Write Protect is off
>  sdc: sdc1 sdc2 sdc3 sdc4

What do you mean by "floppy as second partition"?

> Find appended a patch that does the 0x00/0x80 "boot flag" checks. Please
> discuss and consider for inclusion into the kernel.

> +#define BOOT_IND(p)	(get_unaligned(&p->boot_ind))
>  #define SYS_IND(p)	(get_unaligned(&p->sys_ind))

Hmm. get_unaligned() for a single byte?
I see no reason for these two macros.
Also, it is a good habit to parenthesize macro parameters.

> +	/* 
> +	   Some consistancy check for a valid partition table

consistency

> +	   Boot indicator must either be 0x80 or 0x0 on all primary partitions
> +	   Only one partition may be marked bootable (0x80)
> +	*/
> + 	p = (struct partition *) (data + 0x1be);
> +	for (slot = 1 ; slot <= 4 ; slot++, p++) {
> +	  if ((BOOT_IND(p) != 0x80) && (BOOT_IND(p) != 0x0))
> +	    return 0;
> +	  if (BOOT_IND(p) == 0x80) 
> +	    nr_bootable++;
> +	}
> +	if (nr_bootable > 1) 
> +	  return 0;

I have no objections.

Does it in your case suffice to check for 0 / 0x80 only
(without testing nr_bootable)?

I would prefer to omit that test, until there is at least one
person who shows a boot sector where it is needed.

Andries

Re: rfc: test whether a device has a partition table

[Uwe Bonnes]

>>>>> "Andries" == Andries Brouwer <Andries.Brouwer@cwi.nl> writes:


    Andries> What do you mean by "floppy as second partition"?

Sorry, I mean as second device realized in the stick. On my R50 Laptop
without a floppy drive, when the USB stick is plugged in, it appears as
Floppy "A:" in the boot process.


    >> Find appended a patch that does the 0x00/0x80 "boot flag"
    >> checks. Please discuss and consider for inclusion into the kernel.

    >> +#define BOOT_IND(p) (get_unaligned(&p->boot_ind)) #define SYS_IND(p)
    >> (get_unaligned(&p->sys_ind))

    Andries> Hmm. get_unaligned() for a single byte?  I see no reason for
    Andries> these two macros.  Also, it is a good habit to parenthesize
    Andries> macro parameters.

I must admit that I didn't dig deeper what "get_unaligned" really means. From
what you tell, I understand that the macro is not needed, and the compare
would do if ((&p->sys_ind != 0x80) && (&p->sys_ind != 0x0)) should work too.

    >> + /* + Some consistancy check for a valid partition table

...
    Andries> I have no objections.

    Andries> Does it in your case suffice to check for 0 / 0x80 only
    Andries> (without testing nr_bootable)?

Yes, the test for 0x80/0 is sufficant. Testing nr_bootable was only paranoid...

    Andries> I would prefer to omit that test, until there is at least one
    Andries> person who shows a boot sector where it is needed.

Find appeneded the revised patch.

-- 
Uwe Bonnes                bon@elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. 06151 162516 -------- Fax. 06151 164321 ----------
--- linux-2.6.6/fs/partitions/msdos-sav.c	2004-05-10 04:32:52.000000000 +0200
+++ linux-2.6.6/fs/partitions/msdos.c	2004-05-22 17:14:08.000000000 +0200
@@ -389,6 +389,17 @@
 		put_dev_sector(sect);
 		return 0;
 	}
+
+	/* 
+	   Some consistancy check for a valid partition table
+	   Boot indicator must either be 0x80 or 0x0 on all primary partitions
+	*/
+ 	p = (struct partition *) (data + 0x1be);
+	for (slot = 1 ; slot <= 4 ; slot++, p++) {
+	  if ( (p->boot_ind != 0x80) &&  (p->boot_ind!= 0x0))
+	    return 0;
+	}
+
 	p = (struct partition *) (data + 0x1be);
 #ifdef CONFIG_EFI_PARTITION
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {

[PATCH] Re: rfc: test whether a device has a partition table

[Andries Brouwer]

On Sat, May 22, 2004 at 05:14:24PM +0200, Uwe Bonnes wrote:

> Yes, the test for 0x80/0 is sufficant.

Good.

> +	  if ( (p->boot_ind != 0x80) &&  (p->boot_ind!= 0x0))
> +	    return 0;

You'll need a "put_dev_sector(sect);" as well. Say,

--- msdos.c~	2003-12-18 03:58:58
+++ msdos.c	2004-05-22 19:38:00
@@ -389,8 +389,23 @@
 		put_dev_sector(sect);
 		return 0;
 	}
+
+	/*
+	 * Now that the 55aa signature is present, this is probably
+	 * either the boot sector of a FAT filesystem or a DOS-type
+	 * partition table. Reject this in case the boot indicator
+	 * is not 0 or 0x80.
+	 */
 	p = (struct partition *) (data + 0x1be);
+	for (slot = 1; slot <= 4; slot++, p++) {
+		if (p->boot_ind != 0 && p->boot_ind != 0x80) {
+			put_dev_sector(sect);
+			return 0;
+		}
+	}
+
 #ifdef CONFIG_EFI_PARTITION
+	p = (struct partition *) (data + 0x1be);
 	for (slot = 1 ; slot <= 4 ; slot++, p++) {
 		/* If this is an EFI GPT disk, msdos should ignore it. */
 		if (SYS_IND(p) == EFI_PMBR_OSTYPE_EFI_GPT) {
@@ -398,8 +413,8 @@
 			return 0;
 		}
 	}
-	p = (struct partition *) (data + 0x1be);
 #endif
+	p = (struct partition *) (data + 0x1be);
 
 	/*
 	 * Look for partitions in two passes:

Andries


[Linus, Andrew - I have no objections against this.]