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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZIufZn+reW0rza1H@equinox>
Date:   Fri, 16 Jun 2023 00:31:50 +0100
From:   Phillip Potter <phil@...lpotter.co.uk>
To:     pawan.kumar.gupta@...ux.intel.com
Cc:     linux-kernel@...r.kernel.org, jordyzomer@...gle.com,
        linux-block@...r.kernel.org
Subject: Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget

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@...gle.com>
> > ---
> >  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