[PATCH 0/1] *** CD-ROM: LBA bound check in mmc_ioctl_cdrom_read_data ***

[PATCH 0/1] *** CD-ROM: LBA bound check in mmc_ioctl_cdrom_read_data ***

[Felix Busch]

This introduces an upper-bound check for the Logical Block Address (LBA) in mmc_ioctl_cdrom_read_data 
within the CD-ROM driver. The change is motivated by two main reasons: first, there is currently no 
upper-bound check in place; second, adding this check can help improve execution performance.

There has already been a prior discussion on this topic about this check in mmc_ioctl_cdrom_read_data, 
but this patch follows a slight different approach, due a missunderstading in the SCSI-2 
documentation. (https://www.staff.uni-mainz.de/tacke/scsi/SCSI2-14.html)

Blocks are addressed here using a zero-based integer index. Therefore, it should be possible to check, 
that the LBA falls within the range 0 to N − 1, where N represents the total number of available blocks.

To get the number of blocks in mmc_ioctl_cdrom_read_data, a small inline function
has been added to access the capacity field of the scsi_cd struct, which already contains the
size in blocks available on the CD-ROM. The main reason for this addition is, that
it might be more performant accessing the value there, rather than recalculating the number 
of blocks again in mmc_ioctl_cdrom_read_data. Maybe there's another possibility of doing that, 
which I'm not aware of yet.

While examining the CD-ROM capacity, I noticed, that get_sectorsize changes the capacity value
by multiplying the current set value with four.
```
cd->capacity *= 4;
```
On the tested CD-ROM hardware, keeping this multiplication resulted in an incorrect CD-ROM capacity being reported.
With the line disabled, the capacity appears to be accurate. However, I'm not 100% sure whether this adjustment
may affect other hardware that rely on the original behavior.

One benefit of having an upper bound check for the LBA might be the execution duration of mmc_ioctl_cdrom_read_data.
With this check applied, mmc_ioctl_cdrom_read_data took an average of 0.2217 milliseconds, compared to 6.006 milliseconds
without the check. This performance improvement was observed specifically when the LBA exceeded the number of available
blocks, and the CD-ROM contained actual written data.

Thank you for your time.

Felix Busch (1):
CD-ROM: Additional LBA bound check

drivers/cdrom/cdrom.c |  7 +++++--
drivers/scsi/sr.c     | 12 +++++++++++-
include/linux/cdrom.h |  2 ++
3 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.53.0

[PATCH 1/1] CD-ROM: Additional LBA bound check

[Felix Busch]

Upper bound check for the logical block address
in mmc_ioctl_cdrom_read_data() of the CD-ROM driver.
This prevents trying to read a block when the LBA is
greater than the number of available blocks.

Signed-off-by: Felix Busch <felix.busch1@gmx.net>
---
 drivers/cdrom/cdrom.c |  7 +++++--
 drivers/scsi/sr.c     | 12 +++++++++++-
 include/linux/cdrom.h |  2 ++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index fc049612d6dc..cc0a6c0ae9e7 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2926,6 +2926,8 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
 {
 	struct scsi_sense_hdr sshdr;
 	struct cdrom_msf msf;
+	const struct cdrom_device_ops *cdo = cdi->ops;
+	u64 nr_blocks;
 	int blocksize = 0, format = 0, lba;
 	int ret;
 
@@ -2944,8 +2946,9 @@ static noinline int mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
 	if (copy_from_user(&msf, (struct cdrom_msf __user *)arg, sizeof(msf)))
 		return -EFAULT;
 	lba = msf_to_lba(msf.cdmsf_min0, msf.cdmsf_sec0, msf.cdmsf_frame0);
-	/* FIXME: we need upper bound checking, too!! */
-	if (lba < 0)
+	nr_blocks = cdo->get_capacity(cdi);
+	/* Lower and upper bound check for logical block address. */
+	if ((lba < 0) || (lba > nr_blocks - 1))
 		return -EINVAL;
 
 	cgc->buffer = kzalloc(blocksize, GFP_KERNEL);
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 7adb2573f50d..a056c72341c4 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -120,6 +120,8 @@ static int sr_packet(struct cdrom_device_info *, struct packet_command *);
 static int sr_read_cdda_bpc(struct cdrom_device_info *cdi, void __user *ubuf,
 		u32 lba, u32 nr, u8 *last_sense);
 
+static u64 sr_get_nr_blocks(struct cdrom_device_info *cdi);
+
 static const struct cdrom_device_ops sr_dops = {
 	.open			= sr_open,
 	.release	 	= sr_release,
@@ -134,6 +136,7 @@ static const struct cdrom_device_ops sr_dops = {
 	.audio_ioctl		= sr_audio_ioctl,
 	.generic_packet		= sr_packet,
 	.read_cdda_bpc		= sr_read_cdda_bpc,
+	.get_capacity		= sr_get_nr_blocks,
 	.capability		= SR_CAPABILITIES,
 };
 
@@ -142,6 +145,13 @@ static inline struct scsi_cd *scsi_cd(struct gendisk *disk)
 	return disk->private_data;
 }
 
+static inline u64 sr_get_nr_blocks(struct cdrom_device_info *cdi)
+{
+	struct scsi_cd *cd = scsi_cd(cdi->disk);
+
+	return cd->capacity;
+}
+
 static int sr_runtime_suspend(struct device *dev)
 {
 	struct scsi_cd *cd = dev_get_drvdata(dev);
@@ -782,7 +792,7 @@ static int get_sectorsize(struct scsi_cd *cd)
 			sector_size = 2048;
 			fallthrough;
 		case 2048:
-			cd->capacity *= 4;
+			//cd->capacity *= 4;
 			fallthrough;
 		case 512:
 			break;
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index b907e6c2307d..406e6f4a55bb 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -91,6 +91,8 @@ struct cdrom_device_ops {
 			       struct packet_command *);
 	int (*read_cdda_bpc)(struct cdrom_device_info *cdi, void __user *ubuf,
 			       u32 lba, u32 nframes, u8 *last_sense);
+	/* Get size in blocks */
+	u64 (*get_capacity)(struct cdrom_device_info *cdi);
 /* driver specifications */
 	const int capability;   /* capability flags */
 };
-- 
2.53.0

Re: [PATCH 1/1] CD-ROM: Additional LBA bound check

[James Bottomley]

On Wed, 2026-03-25 at 08:44 +0100, Felix Busch wrote:
[...]
> mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
>  	if (copy_from_user(&msf, (struct cdrom_msf __user *)arg,
> sizeof(msf)))
>  		return -EFAULT;
>  	lba = msf_to_lba(msf.cdmsf_min0, msf.cdmsf_sec0,
> msf.cdmsf_frame0);
> -	/* FIXME: we need upper bound checking, too!! */
> -	if (lba < 0)
> +	nr_blocks = cdo->get_capacity(cdi);

Since you only give sr.c a get_capacity method, doesn't this crash for
every other CD backend?

[...]

> @@ -782,7 +792,7 @@ static int get_sectorsize(struct scsi_cd *cd)
>  			sector_size = 2048;
>  			fallthrough;
>  		case 2048:
> -			cd->capacity *= 4;
> +			//cd->capacity *= 4;
>  			fallthrough;

You mentioned this in the cover letter.  It's because of the
		set_capacity(cd->disk, cd->capacity); below.  The block layer always speaks512bytesectorsforcapacities.Sothiswillbeabreakingchangeforlargesectordevicesbecausethey'llappearfourtimessmallertotheblocklayer.

I suppose I'm also a bit hazy about the actual justification.  Today if
you send an over capacity request via the ioctl, you'll get an error
from the device.  If we apply the patch you'll get a -EINVAL instead. 
So you get an error in both cases (i.e. the macro behaviour doesn't
change, so it's hard to justify why we need the patch) but a different
one, which might confuse some tools ... have you checked?

The cover letter says "improve execution performance" but I can't see
how introducing an indirect call into the read path can do that.  I
can't actually see why you need an indirect call since the capacity
doesn't change except if the device is writeable and rewritten.

Regards,

James

Re: [PATCH 1/1] CD-ROM: Additional LBA bound check

[Felix Busch]

On Wed, Mar 25, 2026 at 08:55:24AM -0400, James Bottomley wrote:
> On Wed, 2026-03-25 at 08:44 +0100, Felix Busch wrote:
> [...]
> > mmc_ioctl_cdrom_read_data(struct cdrom_device_info *cdi,
> >  	if (copy_from_user(&msf, (struct cdrom_msf __user *)arg,
> > sizeof(msf)))
> >  		return -EFAULT;
> >  	lba = msf_to_lba(msf.cdmsf_min0, msf.cdmsf_sec0,
> > msf.cdmsf_frame0);
> > -	/* FIXME: we need upper bound checking, too!! */
> > -	if (lba < 0)
> > +	nr_blocks = cdo->get_capacity(cdi);
> 
> Since you only give sr.c a get_capacity method, doesn't this crash for
> every other CD backend?
> 
> [...]
> 
> > @@ -782,7 +792,7 @@ static int get_sectorsize(struct scsi_cd *cd)
> >  			sector_size = 2048;
> >  			fallthrough;
> >  		case 2048:
> > -			cd->capacity *= 4;
> > +			//cd->capacity *= 4;
> >  			fallthrough;
> 
> You mentioned this in the cover letter.  It's because of the
> 		set_capacity(cd->disk, cd->capacity); below.  The block layer always speaks512bytesectorsforcapacities.Sothiswillbeabreakingchangeforlargesectordevicesbecausethey'llappearfourtimessmallertotheblocklayer.
> 
> I suppose I'm also a bit hazy about the actual justification.  Today if
> you send an over capacity request via the ioctl, you'll get an error
> from the device.  If we apply the patch you'll get a -EINVAL instead. 
> So you get an error in both cases (i.e. the macro behaviour doesn't
> change, so it's hard to justify why we need the patch) but a different
> one, which might confuse some tools ... have you checked?
> 
> The cover letter says "improve execution performance" but I can't see
> how introducing an indirect call into the read path can do that.  I
> can't actually see why you need an indirect call since the capacity
> doesn't change except if the device is writeable and rewritten.
> 
> Regards,
> 
> James
> 
> 
My intension was to fail as early as possible instead of relying on
the device to reject. However, as you pointed out, this does not really
change the overall macro behaviour. Given that, the benefit appears
limited, but it results in an additional error handling. The execution
performance argument relates to the fact that when it's possible to
return, due to wrong input, there's no reason to continue processing
and waiting for the device to reject. Maybe it's fine to leave it was it
is, unless someone as another idea about this.

Regards