NetBSD Problem Report #47676

From www@NetBSD.org  Thu Mar 21 11:13:14 2013
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 4A1A663E6F8
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 21 Mar 2013 11:13:14 +0000 (UTC)
Message-Id: <20130321111313.474AE63E6F8@www.NetBSD.org>
Date: Thu, 21 Mar 2013 11:13:13 +0000 (UTC)
From: markk@clara.co.uk
Reply-To: markk@clara.co.uk
To: gnats-bugs@NetBSD.org
Subject: Off-by-one error in reported CD capacity means last sector cannot be read
X-Send-Pr-Version: www-1.0

>Number:         47676
>Category:       kern
>Synopsis:       Off-by-one error in reported CD capacity means last sector cannot be read
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    reinoud
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Mar 21 11:15:00 +0000 2013
>Last-Modified:  Thu May 02 17:30:06 +0000 2013
>Originator:     Mark
>Release:        Any after 4.0
>Organization:
>Environment:
>Description:
There is an off-by-one bug in NetBSD's CD capacity code.

For example, doing
  dd if=/dev/rcd0c of=my_cd_image.iso bs=2048
creates an image file which has the last sector missing. Also doing
  md5 /dev/rcd0c
to generate a checksum of a just-burned CD to compare with a published checksum will fail.

I have been testing various versions of NetBSD/amiga with the WinUAE Amiga emulator. In the course of that, I discovered this bug which appeared somewhere between NetBSD 3.1.1 and NetBSD 4.0.

The bug is in the read_cd_capacity() function in src/sys/dev/scsipi/cd.c
The bug first appeared in version 1.227. The corresponding function name in earlier versions was cd_size().
Compare line 1662 of version 1.226 with line 1643 of version 1.227:
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/scsipi/cd.c?annotate=1.226
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/scsipi/cd.c?annotate=1.227

1.226 line 1662:
       size = _4btol(data.addr) + 1;

1.227 line 1643:
       *size = _4btol(data.addr);

>How-To-Repeat:
Insert a CD-ROM. Use dd to read it and notice the last sector is omitted from the image file. Do the same with NetBSD 3.1 (or another OS) and it works correctly.
>Fix:
In the current version 1.310 of src/sys/dev/scsipi/cd.c the problem line is 1821. Change that from
        *size    = _4btol(cap.addr);
to
        *size    = _4btol(cap.addr) + 1;

>Release-Note:

>Audit-Trail:
From: mlelstv@serpens.de (Michael van Elst)
To: gnats-bugs@netbsd.org
Cc: 
Subject: Re: kern/47676: Off-by-one error in reported CD capacity means last sector cannot be read
Date: Thu, 21 Mar 2013 17:28:31 +0000 (UTC)

 Seems to be slightly more complex.

 read_cd_capacity(struct scsipi_periph *periph, u_int *blksize, u_long *size)
 error = read_cd_capacity(cd->sc_periph, &blksize, &size);
 error = read_cd_capacity(cd->sc_periph, &blksize, &size);
 error = read_cd_capacity(periph, &mmc_discinfo->sector_size, &last_lba);

 One time the caller apparently expects the last_lba value as is returned
 by the READ RECORDED CD CAPACITY command.

 -- 
 -- 
                                 Michael van Elst
 Internet: mlelstv@serpens.de
                                 "A potential Snark may lurk in every tree."

From: "Mark" <markk@clara.co.uk>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: kern/47676: Off-by-one error in reported CD capacity means last
 sector cannot be read
Date: Thu, 21 Mar 2013 17:57:21 -0000

 On Thu, March 21, 2013 17:30, Michael van Elst wrote:
 > The following reply was made to PR kern/47676; it has been noted by GNATS.
 >
 > From: mlelstv@serpens.de (Michael van Elst)
 > To: gnats-bugs@netbsd.org
 > Cc:
 > Subject: Re: kern/47676: Off-by-one error in reported CD capacity means
 > last sector cannot be read
 > Date: Thu, 21 Mar 2013 17:28:31 +0000 (UTC)
 >
 >  Seems to be slightly more complex.
 >
 >  read_cd_capacity(struct scsipi_periph *periph, u_int *blksize, u_long
 > *size)
 >  error = read_cd_capacity(cd->sc_periph, &blksize, &size);
 >  error = read_cd_capacity(cd->sc_periph, &blksize, &size);
 >  error = read_cd_capacity(periph, &mmc_discinfo->sector_size, &last_lba);
 >
 >  One time the caller apparently expects the last_lba value as is returned
 >  by the READ RECORDED CD CAPACITY command.

 I don't think the caller does, though the last_lba variable name is quite
 misleading.

 The caller is the mmc_getdiscinfo() function in src/sys/dev/scsipi/cd.c.
 Code around line 3005 in the NetBSD 6.0.1 source:
 	error = read_cd_capacity(periph, &mmc_discinfo->sector_size, &last_lba);
 	if (error)
 		goto out;

 	mmc_discinfo->last_possible_lba = (uint32_t) last_lba - 1;

 So last_possible_lba is set assuming that the "last_lba" variable is
 actually the total number of sectors, i.e. the number of the last LBA + 1.
 No code change is needed there, though someone might want to rename
 last_lba to be less confusing.



Responsible-Changed-From-To: kern-bug-people->reinoud
Responsible-Changed-By: reinoud@NetBSD.org
Responsible-Changed-When: Thu, 21 Mar 2013 21:09:51 +0000
Responsible-Changed-Why:
I'm taking it and am testing out a patch


From: Reinoud Zandijk <reinoud@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, markk@clara.co.uk
Subject: Re: kern/47676: Off-by-one error in reported CD capacity means last
 sector cannot be read
Date: Thu, 21 Mar 2013 22:05:23 +0100

 On Thu, Mar 21, 2013 at 05:30:10PM +0000, Michael van Elst wrote:
 >  Seems to be slightly more complex.
 >  
 >  read_cd_capacity(struct scsipi_periph *periph, u_int *blksize, u_long *size)
 >  error = read_cd_capacity(cd->sc_periph, &blksize, &size);
 >  error = read_cd_capacity(cd->sc_periph, &blksize, &size);
 >  error = read_cd_capacity(periph, &mmc_discinfo->sector_size, &last_lba);
 >  
 >  One time the caller apparently expects the last_lba value as is returned
 >  by the READ RECORDED CD CAPACITY command.

 I'll take this PR# since its my code patch :) and yes, the result is being
 used ambigiously and thats not good. I am preparing a patch to clean this up
 and streamline the code again.

 With regards,
 Reinoud

From: Reinoud Zandijk <reinoud@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, markk@clara.co.uk
Subject: Re: kern/47676: Off-by-one error in reported CD capacity means last
 sector cannot be read
Date: Thu, 28 Mar 2013 19:07:29 +0100

 The real reason of the error might not even be in NetBSD's cd.c itself though
 i'm preparing a patch to cleanup things.

 What is going on is that the device is lying to you! The on disk low level CD
 format is quite overcomplex complete with sector data interleaving etc. it
 was also originally designed for audio.

 What can be done is on detection of a CD-R, adjust the size of the tracks as
 per definition of mmc; i might need to add 2 sectors.


From: "Mark" <markk@clara.co.uk>
To: "Reinoud Zandijk" <reinoud@NetBSD.org>
Cc: gnats-bugs@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 markk@clara.co.uk
Subject: Re: kern/47676: Off-by-one error in reported CD capacity means last
 sector cannot be read
Date: Thu, 28 Mar 2013 19:08:18 -0000

 On Thu, March 28, 2013 18:07, Reinoud Zandijk wrote:
 > The real reason of the error might not even be in NetBSD's cd.c itself
 > though i'm preparing a patch to cleanup things.
 >
 > What is going on is that the device is lying to you! The on disk low level
 > CD
 > format is quite overcomplex complete with sector data interleaving etc. it
 > was also originally designed for audio.
 >
 > What can be done is on detection of a CD-R, adjust the size of the tracks
 > as per definition of mmc; i might need to add 2 sectors.

 When I noticed the bug, I was using an emulated CD drive, so there were no
 strange real-CD-drive-related issues.

 I read the SCSI-2 spec (s2-r10l.pdf) READ CD-ROM CAPACITY description (on
 page 313, PDF page 349). That says:
 "The logical block address returned shall be greater than or equal to the
 last readable or playable block. If greater, this address may be in a
 transition area beyond the last valid logical block for read or audio play
 operations. The value returned shall not be not be more than 75 sectors
 (MSF units) greater than the logical block address of the last readable or
 playable block. (This value arises because the CD-ROM table of contents
 lead-out track location has a +/- 75 sector tolerance when the lead-out
 track is encoded as an audio track.)"

 So essentially, for CD drives compliant with the old SCSI-2 spec, the last
 LBA value returned by READ CAPACITY is equal or greater to the "real" last
 LBA value.

 For some drives/discs that would mean getting an error message near the
 end if you try to do
   dd if=/dev/rcd0c of=image.iso bs=2048
 But at least all readable sectors would be read.

 The later MMC spec (I looked at ftp://ftp.seagate.com/sff/INF-8090.PDF
 page 783) doesn't allow the returned highest LBA to be greater than the
 actual highest LBA for non-CD media. It has this to say about CD media:
 "For CD media, the logical unit shall use the AAh point found in the last
 Table of Contents, convert to an LBA, and subtract one. If that block is a
 run-out block (found on incrementally recorded CD-R and CD-RW), the
 logical unit shall subtract two."

 Can run-out blocks (whatever they are) on incrementally recorded CD-R/W be
 read? Because it seems that that's the only case where adding 2 could be
 of any use. In all other cases the highest LBA reported by the drive is
 equal to or greater than the actual highest accessible LBA.


 -- Mark


From: Reinoud Zandijk <reinoud@NetBSD.org>
To: Mark <markk@clara.co.uk>
Cc: gnats-bugs@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/47676: Off-by-one error in reported CD capacity means last
 sector cannot be read
Date: Fri, 29 Mar 2013 23:02:26 +0100

 On Thu, Mar 28, 2013 at 07:08:18PM -0000, Mark wrote:
 > For some drives/discs that would mean getting an error message near the
 > end if you try to do
 >   dd if=/dev/rcd0c of=image.iso bs=2048
 > But at least all readable sectors would be read.
 True, and answering your later question on run-out blocks: no they are
 adressable but NOT readable, a big mistake IMHO that they made.

 Run-out blocks are the blocks that are needed to fill up the 7 (IIRC) sector
 lower level blocking that goes on in CDs. For CD-RW its 32 and for DVD-* its
 16. Run-out sectors themselves on CD-R(W) can't be read but the few sectors
 before them in the packet are.

 > The later MMC spec (I looked at ftp://ftp.seagate.com/sff/INF-8090.PDF
 > page 783) doesn't allow the returned highest LBA to be greater than the
 > actual highest LBA for non-CD media. It has this to say about CD media:
 > "For CD media, the logical unit shall use the AAh point found in the last
 > Table of Contents, convert to an LBA, and subtract one. If that block is a
 > run-out block (found on incrementally recorded CD-R and CD-RW), the
 > logical unit shall subtract two."

 BRRR... the horrors haunt me again :) They seem to always create a -ROM first
 and then later bodge -R and -RW/-RE versions *sigh*

 It would explain the 2 less reported. Hmm i could make the code check the disc
 type and if its a CD-R(W) make it DTRT but it won't be pretty. On recorders I
 use the newer READ_TRACKINFO but aparently they haven't fixed it there either,
 at least in some drives.

 It needs more thought aparently, though i hope to fix it ASAP before 6.2 gets
 rolled out or 7 gets forked.

From: "Mark" <markk@clara.co.uk>
To: gnats-bugs@NetBSD.org
Cc: reinoud@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org
Subject: Re: kern/47676: Off-by-one error in reported CD capacity means last
 sector cannot be read
Date: Thu, 2 May 2013 17:11:50 +0100

 On Fri, March 29, 2013 23:05, Reinoud Zandijk wrote:
 >  BRRR... the horrors haunt me again :) They seem to always create a -ROM
 > first and then later bodge -R and -RW/-RE versions *sigh*
 >
 >  It would explain the 2 less reported. Hmm i could make the code check the
 > disc
 >  type and if its a CD-R(W) make it DTRT but it won't be pretty. On
 > recorders I
 >  use the newer READ_TRACKINFO but aparently they haven't fixed it there
 > either,
 >  at least in some drives.
 >
 >  It needs more thought aparently, though i hope to fix it ASAP before 6.2
 > gets
 >  rolled out or 7 gets forked.

 I wonder whether it's worth trying to correct too-large reported
 capacities at all. Do any other operating systems attempt to do that? It
 may not be worth the hassle.

 Many pressed CD-ROMs cause drives to report a too-large capacity as well.

>Unformatted:

Home	PR Database Search