[PATCH] sun disk label: fix signed int usage for sector count

[PATCH] sun disk label: fix signed int usage for sector count

[Jurzitza, Dieter]

Dear listmembers,
citing Jeff Mahoney:

> I'm not sure how many people out there are using Sun disk labels with sizes > 1TB.
> It seems like a pretty rare corner case, but there's no reason any data stored in
> those partitions wouldn't be invalid, and it will suddenly cut them off. Is this a
> rare enough occurrence that we don't care?

I am one of those guys using Sun disklabels with sizes > 1TB. And I would highly appreciate if nobody out there would make my disk drive useless by simply discarding anything larger than 1 TB. This would render our department server useless, what is everything but desirable :-)

Fdisk says:

Kommando (m für Hilfe): 
Disk /dev/sda (Sun disk label): 1024 heads, 63 sectors, 36187 cylinders
Units = Zylinder of 64512 * 512 bytes

    Gerät Flag    Start       End    Blocks   Id  System
/dev/sda1             0         1     32256   83  Linux native
/dev/sda2  u          1        64   2032128   82  Linux Swap
/dev/sda3             0     36187 -980235776    5  Whole disk
/dev/sda4            64       320   8257536    1  Boot
/dev/sda5           320      4864 146571264   83  Linux native
/dev/sda6          4864     16384 371589120   83  Linux native
/dev/sda7         16384     36187 638765568   83  Linux native

Kommando (m für Hilfe): 

cat /proc/partitions says:
/home/fred> cat /proc/partitions 
major minor  #blocks  name

   8     0 1171874880 sda
   8     1      32256 sda1
   8     2    2032128 sda2
   8     3 -980235776 sda3
   8     4    8257536 sda4
   8     5  146571264 sda5
   8     6  371589120 sda6
   8     7  638765568 sda7

but do not leave sda3 useless by changing the code. If there would be a chance to make fdisk and /proc/partitions not to display negative numbers (since, though obvious, they are nonsense) it would be preferrable.

Many thanks for your help,
take care


Dieter Jurzitza

-- 
________________________________________________

HARMAN BECKER AUTOMOTIVE SYSTEMS

Dr.-Ing. Dieter Jurzitza
Manager Hardware Systems
   System Development

Industriegebiet Ittersbach
Becker-Göring Str. 16
D-76307 Karlsbad / Germany

Phone: +49 (0)7248 71-1577
Fax:   +49 (0)7248 71-1216
eMail: DJurzitza@harmanbecker.com
Internet: http://www.becker.de
 


*******************************************
Diese E-Mail enthaelt vertrauliche und/oder rechtlich geschuetzte Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtuemlich erhalten haben, informieren Sie bitte sofort den Absender und loeschen Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail ist nicht gestattet.
 
This e-mail may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and delete this e-mail. Any unauthorized copying, disclosure or distribution of the contents in this e-mail is strictly forbidden.
*******************************************

Re: [PATCH] sun disk label: fix signed int usage for sector count

[David Miller]

From: Jeff Mahoney <jeffm@suse.com>
Date: Sat, 26 Aug 2006 12:02:46 -0400

> You're right. The Solaris side of things treats this as signed.

I see no reason to disallow this and use an unsigned quantity.

Re: [PATCH] sun disk label: fix signed int usage for sector count

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jan Engelhardt wrote:
>> The current sun disklabel code uses a signed int for the sector count. When
>> partitions larger than 1 TB are used, the cast to a sector_t causes the
>> partition sizes to be invalid:
>>
> 
> Is not it that the sun disklabel does not even support 
> [ptabs/partitions] more than 1 TB?

You're right. The Solaris side of things treats this as signed. I'm not sure how
the user reporting this problem ended up with partitions larger than 1 TB. It
looks like Solaris just treats the entire label as invalid and rejects the whole thing.

I'm not sure how many people out there are using Sun disk labels with sizes > 1TB.
It seems like a pretty rare corner case, but there's no reason any data stored in
those partitions wouldn't be invalid, and it will suddenly cut them off. Is this a
rare enough occurrence that we don't care?

The following patch rejects the partitions, but it would be trivial to switch it
to a warning. I'll post it separately if we agree this is the correct solution.

- -Jeff

 Solaris treats Sun disklabel partition sizes as a signed int, and rejects
 partitions that claim to be larger than 1 TB.

 Linux likewise treats the sector count as a signed int, but doesn't do
 any checking, and passes it to put_partition, which casts it causing it to
 be sign extended.

 This patch performs checking to see if individual partitions are valid, and
 rejects the disk label entirely if they are not.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>

- --- linux-2.6.17/fs/partitions/sun.c	2006-01-02 22:21:10.000000000 -0500
+++ linux-2.6.17.fix/fs/partitions/sun.c	2006-08-26 11:54:54.000000000 -0400
@@ -74,10 +74,18 @@ int sun_partition(struct parsed_partitio
 	spc = be16_to_cpu(label->ntrks) * be16_to_cpu(label->nsect);
 	for (i = 0; i < 8; i++, p++) {
 		unsigned long st_sector;
- -		int num_sectors;
+		unsigned int num_sectors;
 
 		st_sector = be32_to_cpu(p->start_cylinder) * spc;
 		num_sectors = be32_to_cpu(p->num_sectors);
+
+		if (num_sectors > INT_MAX) {
+			printk("Dev %s Sun disklabel: partition %d has "
+			       "invalid size > 1 TB\n", bdevname(bdev, b), i);
+			put_dev_sector(sec);
+			return 0;
+		}
+
 		if (num_sectors) {
 			put_partition(state, slot, st_sector, num_sectors);
 			if (label->infos[i].id == LINUX_RAID_PARTITION)


- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFE8HCmLPWxlyuTD7IRAiqZAJ0beMj4oMVsFv0T7RQ1cok751MfCQCfZuAW
M5hesfVBbgyUmpmzsYBf06w=
=o35O
-----END PGP SIGNATURE-----

Re: [PATCH] sun disk label: fix signed int usage for sector count

[Jan Engelhardt]

>
> The current sun disklabel code uses a signed int for the sector count. When
> partitions larger than 1 TB are used, the cast to a sector_t causes the
> partition sizes to be invalid:
>

Is not it that the sun disklabel does not even support 
[ptabs/partitions] more than 1 TB?


Jan Engelhardt
-- 

[PATCH] sun disk label: fix signed int usage for sector count

[Jeff Mahoney]

 The current sun disklabel code uses a signed int for the sector count. When
 partitions larger than 1 TB are used, the cast to a sector_t causes the
 partition sizes to be invalid:

 # cat /proc/paritions | grep sdan
   66   112 2146435072 sdan
   66   115 9223372036853660736 sdan3
   66   120 9223372036853660736 sdan8

 This patch switches the sector count to an unsigned int to fix this.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>

--- linux-2.6.5/fs/partitions/sun.c	2004-04-03 22:38:14.000000000 -0500
+++ linux-2.6.5.fix/fs/partitions/sun.c	2006-08-25 11:54:23.000000000 -0400
@@ -73,7 +73,7 @@
 	spc = be16_to_cpu(label->ntrks) * be16_to_cpu(label->nsect);
 	for (i = 0; i < 8; i++, p++) {
 		unsigned long st_sector;
-		int num_sectors;
+		unsigned int num_sectors;
 
 		st_sector = be32_to_cpu(p->start_cylinder) * spc;
 		num_sectors = be32_to_cpu(p->num_sectors);

-- 
Jeff Mahoney
SUSE Labs