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

[Felix Busch <felix.busch1@gmx.net>]

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