NetBSD Problem Report #47081

From msaitoh@execsw.org  Mon Oct 15 14:10:48 2012
Return-Path: <msaitoh@execsw.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 2674663E059
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 15 Oct 2012 14:10:48 +0000 (UTC)
Message-Id: <20121015141046.45F7E439857@vslock.execsw.org>
Date: Mon, 15 Oct 2012 23:10:46 +0900 (JST)
From: msaitoh@execsw.org
Reply-To: msaitoh@execsw.org
To: gnats-bugs@gnats.NetBSD.org
Subject: Wrong offset and size in MBR partition on armeb.
X-Send-Pr-Version: 3.95

>Number:         47081
>Category:       port-arm
>Synopsis:       Wrong offset and size in MBR partition on armeb.
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    msaitoh
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Oct 15 14:15:00 +0000 2012
>Closed-Date:    Thu Nov 08 06:31:51 +0000 2012
>Last-Modified:  Thu Nov 08 06:31:51 +0000 2012
>Originator:     SAITOH Masanobu
>Release:        all releases
>Organization:
>Environment:
1.6 and newer.
Architecture: armeb
Machine: evbarm
>Description:
	arm/arm/disksubr_mbr.c has a long standing bug that the offset
	and size of MBR partition table read as host byteorder.

	An example on armeb (NSLU2):
nslu2# disklabel sd0
# /dev/rsd0c:
type: SCSI
disk: USB Flash Disk
label: fictitious-MBR
flags: removable
bytes/sector: 512
sectors/track: 32
tracks/cylinder: 16
sectors/cylinder: 512
cylinders: 492
total sectors: 251904
rpm: 3600
interleave: 1
trackskew: 0
cylinderskew: 0
headswitch: 0           # microseconds
track-to-track seek: 0  # microseconds
drivedata: 0

8 partitions:
#        size    offset     fstype [fsize bsize cpg/sgs]
 c:    251904         0     unused      0     0        # (Cyl.      0 -    491)
 e: 262996736 1056964608      MSDOS                     # (Cyl. 2064384+- 2578049+)
disklabel: boot block size 0
disklabel: super block size 0
disklabel: partition e: offset past end of unit
disklabel: partition e: partition extends past end of unit

>How-To-Repeat:
	Connect a USB disk which has MBR partition (MSDOS formatted)
	and do "disklabel sd0" or something.

>Fix:
	Patch:

Index: disksubr_mbr.c
===================================================================
RCS file: /cvsroot/src/sys/arch/arm/arm/disksubr_mbr.c,v
retrieving revision 1.13
diff -u -r1.13 disksubr_mbr.c
--- disksubr_mbr.c	2 Dec 2011 00:25:37 -0000	1.13
+++ disksubr_mbr.c	15 Oct 2012 14:09:18 -0000
@@ -128,6 +128,9 @@
   	  memcpy(mbrp, (char *)bp->b_data + MBR_PART_OFFSET,
 	  	             MBR_PART_COUNT * sizeof(*mbrp));

+		LE32TOH(mbrp->mbrp_start);
+		LE32TOH(mbrp->mbrp_size);
+
		/* look for NetBSD partition */
 		   ourmbrp = NULL;
 		   	   for (i = 0; !ourmbrp && i < MBR_PART_COUNT; i++) {
@@ -249,6 +252,9 @@
   	  goto out;
 	  }

+	HTOLE32(ourmbrp->mbrp_start);
+	HTOLE32(ourmbrp->mbrp_size);
+
	/* need sector address for SCSI/IDE, cylinder for ESDI/ST506/RLL */
 	mbrpartoff = ourmbrp->mbrp_start;
 	cyl = MBR_PCYL(ourmbrp->mbrp_scyl, ourmbrp->mbrp_ssect);

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: port-arm-maintainer->msaitoh
Responsible-Changed-By: msaitoh@NetBSD.org
Responsible-Changed-When: Mon, 15 Oct 2012 14:18:20 +0000
Responsible-Changed-Why:
mine.


From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: gnats-bugs@NetBSD.org
Cc: port-arm-maintainer@NetBSD.org, gnats-admin@NetBSD.org,
        netbsd-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-arm/47081: Wrong offset and size in MBR partition on armeb.
Date: Tue, 16 Oct 2012 00:01:04 +0900

 > RCS file: /cvsroot/src/sys/arch/arm/arm/disksubr_mbr.c,v
 > +		LE32TOH(mbrp->mbrp_start);
 > +		LE32TOH(mbrp->mbrp_size);

 Isn't it better to "swap on copying between on-disk-format and host-variable"
 to avoid confusion?


 Index: disksubr_mbr.c
 ===================================================================
 RCS file: /cvsroot/src/sys/arch/arm/arm/disksubr_mbr.c,v
 retrieving revision 1.13
 diff -u -p -r1.13 disksubr_mbr.c
 --- disksubr_mbr.c	2 Dec 2011 00:25:37 -0000	1.13
 +++ disksubr_mbr.c	15 Oct 2012 14:58:07 -0000
 @@ -151,23 +151,23 @@ mbr_label_read(dev_t dev,

  			/* Install in partition e, f, g, or h. */
  			pp = &lp->d_partitions['e' - 'a' + i];
 -			pp->p_offset = mbrp->mbrp_start;
 -			pp->p_size = mbrp->mbrp_size;
 +			pp->p_offset = le32toh(mbrp->mbrp_start);
 +			pp->p_size = le32toh(mbrp->mbrp_size);
  			pp->p_fstype = xlat_mbr_fstype(mbrp->mbrp_type);

  			/* is this ours? */
  			if (mbrp == ourmbrp) {
  				/* need sector address for SCSI/IDE,
  				 cylinder for ESDI/ST506/RLL */
 -				mbrpartoff = mbrp->mbrp_start;
 +				mbrpartoff = le32toh(mbrp->mbrp_start);
  				cyl = MBR_PCYL(mbrp->mbrp_scyl, mbrp->mbrp_ssect);

  #ifdef __i386__ /* XXX? */
  				/* update disklabel with details */
  				lp->d_partitions[2].p_size =
 -				    mbrp->mbrp_size;
 +				    le32toh(mbrp->mbrp_size);
  				lp->d_partitions[2].p_offset = 
 -				    mbrp->mbrp_start;
 +				    le32toh(mbrp->mbrp_start);
  				lp->d_ntracks = mbrp->mbrp_ehd + 1;
  				lp->d_nsectors = MBR_PSECT(mbrp->mbrp_esect);
  				lp->d_secpercyl =
 @@ -250,7 +250,7 @@ mbr_label_locate(dev_t dev,
  	}

  	/* need sector address for SCSI/IDE, cylinder for ESDI/ST506/RLL */
 -	mbrpartoff = ourmbrp->mbrp_start;
 +	mbrpartoff = le32toh(ourmbrp->mbrp_start);
  	cyl = MBR_PCYL(ourmbrp->mbrp_scyl, ourmbrp->mbrp_ssect);

  	*cylp = cyl;

From: Masanobu SAITOH <msaitoh@execsw.org>
To: gnats-bugs@NetBSD.org
Cc: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>, msaitoh@NetBSD.org, 
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, msaitoh@execsw.org
Subject: Re: port-arm/47081: Wrong offset and size in MBR partition on armeb.
Date: Tue, 16 Oct 2012 09:21:45 +0900

 (2012/10/16 0:05), Izumi Tsutsui wrote:
 > The following reply was made to PR port-arm/47081; it has been noted by GNATS.
 > 
 > From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
 > To: gnats-bugs@NetBSD.org
 > Cc: port-arm-maintainer@NetBSD.org, gnats-admin@NetBSD.org,
 >          netbsd-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
 > Subject: Re: port-arm/47081: Wrong offset and size in MBR partition on armeb.
 > Date: Tue, 16 Oct 2012 00:01:04 +0900
 > 
 >   > RCS file: /cvsroot/src/sys/arch/arm/arm/disksubr_mbr.c,v
 >   > +		LE32TOH(mbrp->mbrp_start);
 >   > +		LE32TOH(mbrp->mbrp_size);
 >   
 >   Isn't it better to "swap on copying between on-disk-format and host-variable"
 >   to avoid confusion?

  Yes, it is. I agree with you.

  I'll commit with your patch that I've already tested.

  Thanks.



 >   
 >   Index: disksubr_mbr.c
 >   ===================================================================
 >   RCS file: /cvsroot/src/sys/arch/arm/arm/disksubr_mbr.c,v
 >   retrieving revision 1.13
 >   diff -u -p -r1.13 disksubr_mbr.c
 >   --- disksubr_mbr.c	2 Dec 2011 00:25:37 -0000	1.13
 >   +++ disksubr_mbr.c	15 Oct 2012 14:58:07 -0000
 >   @@ -151,23 +151,23 @@ mbr_label_read(dev_t dev,
 >    
 >    			/* Install in partition e, f, g, or h. */
 >    			pp = &lp->d_partitions['e' - 'a' + i];
 >   -			pp->p_offset = mbrp->mbrp_start;
 >   -			pp->p_size = mbrp->mbrp_size;
 >   +			pp->p_offset = le32toh(mbrp->mbrp_start);
 >   +			pp->p_size = le32toh(mbrp->mbrp_size);
 >    			pp->p_fstype = xlat_mbr_fstype(mbrp->mbrp_type);
 >    
 >    			/* is this ours? */
 >    			if (mbrp == ourmbrp) {
 >    				/* need sector address for SCSI/IDE,
 >    				 cylinder for ESDI/ST506/RLL */
 >   -				mbrpartoff = mbrp->mbrp_start;
 >   +				mbrpartoff = le32toh(mbrp->mbrp_start);
 >    				cyl = MBR_PCYL(mbrp->mbrp_scyl, mbrp->mbrp_ssect);
 >    
 >    #ifdef __i386__ /* XXX? */
 >    				/* update disklabel with details */
 >    				lp->d_partitions[2].p_size =
 >   -				    mbrp->mbrp_size;
 >   +				    le32toh(mbrp->mbrp_size);
 >    				lp->d_partitions[2].p_offset =
 >   -				    mbrp->mbrp_start;
 >   +				    le32toh(mbrp->mbrp_start);
 >    				lp->d_ntracks = mbrp->mbrp_ehd + 1;
 >    				lp->d_nsectors = MBR_PSECT(mbrp->mbrp_esect);
 >    				lp->d_secpercyl =
 >   @@ -250,7 +250,7 @@ mbr_label_locate(dev_t dev,
 >    	}
 >    
 >    	/* need sector address for SCSI/IDE, cylinder for ESDI/ST506/RLL */
 >   -	mbrpartoff = ourmbrp->mbrp_start;
 >   +	mbrpartoff = le32toh(ourmbrp->mbrp_start);
 >    	cyl = MBR_PCYL(ourmbrp->mbrp_scyl, ourmbrp->mbrp_ssect);
 >    
 >    	*cylp = cyl;
 >   
 > 


 -- 
 -----------------------------------------------
                 SAITOH Masanobu (msaitoh@execsw.org
                                  msaitoh@netbsd.org)

From: "SAITOH Masanobu" <msaitoh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47081 CVS commit: src/sys/arch/arm/arm
Date: Tue, 16 Oct 2012 00:25:10 +0000

 Module Name:	src
 Committed By:	msaitoh
 Date:		Tue Oct 16 00:25:10 UTC 2012

 Modified Files:
 	src/sys/arch/arm/arm: disksubr_mbr.c

 Log Message:
 Fix a bug that armeb machine misunderstand MBR partition's offset
 and size. Fixes PR#47081


 To generate a diff of this commit:
 cvs rdiff -u -r1.13 -r1.14 src/sys/arch/arm/arm/disksubr_mbr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Jeff Rizzo" <riz@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/47081 CVS commit: [netbsd-6] src/sys/arch/arm/arm
Date: Tue, 23 Oct 2012 16:21:32 +0000

 Module Name:	src
 Committed By:	riz
 Date:		Tue Oct 23 16:21:32 UTC 2012

 Modified Files:
 	src/sys/arch/arm/arm [netbsd-6]: disksubr_mbr.c

 Log Message:
 Pull up following revision(s) (requested by msaitoh in ticket #617):
 	sys/arch/arm/arm/disksubr_mbr.c: revision 1.14
 Fix a bug that armeb machine misunderstand MBR partition's offset
 and size. Fixes PR#47081


 To generate a diff of this commit:
 cvs rdiff -u -r1.13 -r1.13.4.1 src/sys/arch/arm/arm/disksubr_mbr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: msaitoh@NetBSD.org
State-Changed-When: Thu, 08 Nov 2012 06:31:51 +0000
State-Changed-Why:
fixed.


>Unformatted:

Home	PR Database Search