Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget

[Phillip Potter]

On Thu, Jun 15, 2023 at 09:31:25AM -0700, Pawan Gupta wrote:
> On Mon, Jun 12, 2023 at 11:00:40AM +0000, Jordy Zomer wrote:
> > This patch fixes a spectre-v1 gadget in cdrom.
> > The gadget could be triggered by,
> >  speculatviely bypassing the cdi->capacity check.
> > 
> > Signed-off-by: Jordy Zomer <jordyzomer@xxxxxxxxxx>
> > ---
> >  drivers/cdrom/cdrom.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> > index 416f723a2dbb..ecf2b458c108 100644
> > --- a/drivers/cdrom/cdrom.c
> > +++ b/drivers/cdrom/cdrom.c
> > @@ -264,6 +264,7 @@
> >  #include <linux/errno.h>
> >  #include <linux/kernel.h>
> >  #include <linux/mm.h>
> > +#include <linux/nospec.h>
> >  #include <linux/slab.h> 
> >  #include <linux/cdrom.h>
> >  #include <linux/sysctl.h>
> > @@ -2329,6 +2330,9 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
> >  	if (arg >= cdi->capacity)
> >  		return -EINVAL;
> >  
> > +	/* Prevent arg from speculatively bypassing the length check */
> > +	barrier_nospec();
> 
> On a quick look it at the call chain ...
> 
> sr_block_ioctl(..., arg)
>   cdrom_ioctl(..., arg)
>     cdrom_ioctl_media_changed(..., arg)
> 
> .... it appears maximum value cdi->capacity can be only 1:
> 
> sr_probe()
> {
> ...
> 	cd->cdi.capacity = 1;
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/sr.c?h=v6.4-rc6#n665
> 
> If we know that max possible value than, instead of big hammer
> barrier_nospec(), its possible to use lightweight array_index_nospec()
> as below:
> ...

Hi Pawan and Jordy,

I've now looked at this. It is possible for cdi->capacity to be > 1, as
it is set via get_capabilities() -> cdrom_number_of_slots(), if the
device is an individual or cartridge changer.

Therefore, I think using CDI_MAX_CAPACITY of 1 is not the correct
approach. Jordy's V2 patch is fine therefore, but perhaps using
array_index_nospec() with cdi->capacity is still better than a
do/while loop from a performance perspective, given it would be cached
etc. at that point, so possibly quicker. Thoughts? (I'm no expert on
spectre-v1 I'll admit).

Regards,
Phil