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

[Phillip Potter]

Hi Felix,

Nice to hear from you again, hope you're well. Apologies for me taking a
few days to reply, I've had a a fair amount on at work.

On Wed, Mar 25, 2026 at 08:44:01AM +0100, Felix Busch wrote:
> 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@xxxxxxx>
> ---
>  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);

This (as with other function pointers that are part of cdrom_device_ops,
should be checked first to see if it's initialised? (it is for sr.c of course,
but what about others?)

> +	/* Lower and upper bound check for logical block address. */
> +	if ((lba < 0) || (lba > nr_blocks - 1))
>  		return -EINVAL;

The point James makes about the return value now being different (as it
would previously have been an error from the device itself) is a good
one. This is probably the hardest question to answer in terms of which
software may or may not break.

>  
>  	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;

This is here for a reason, it should not be commented out. I may not
have worded it clearly in my response to your previous patch, but this
is the kind of thing I was talking about before.

>  			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);

I notice we only define an implementation of this for sr.c? At a glance,
it's also possible to trigger this code from the GD-ROM driver, unless
I'm missing something?

>  /* driver specifications */
>  	const int capability;   /* capability flags */
>  };
> -- 
> 2.53.0
>

I take your point about avoiding the read entirely, and I see what you
mean there. That said, I'm not sure this is necessary given the
potential dangers.

Regards,
Phil