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
