Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 15 Jun 2023 09:31:25 -0700
From:   Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
To:     Jordy Zomer <jordyzomer@google.com>
Cc:     linux-kernel@vger.kernel.org, phil@philpotter.co.uk
Subject: Re: [PATCH v2 1/1] cdrom: Fix spectre-v1 gadget
Message-ID: <20230615163125.td3aodpfwth5n4mc@desk>
References: <20230612110040.849318-1-jordyzomer@google.com>
 <20230612110040.849318-2-jordyzomer@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230612110040.849318-2-jordyzomer@google.com>
Precedence: bulk

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@google.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:

---
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 416f723a2dbb..e1c4f969ffda 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 */
+	arg = array_index_nospec(arg, CDI_MAX_CAPACITY);
+
 	info = kmalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 12869e6d4ebd..62e163dc29cc 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -662,7 +662,7 @@ static int sr_probe(struct device *dev)
 	cd->cdi.ops = &sr_dops;
 	cd->cdi.handle = cd;
 	cd->cdi.mask = 0;
-	cd->cdi.capacity = 1;
+	cd->cdi.capacity = CDI_MAX_CAPACITY;
 	sprintf(cd->cdi.name, "sr%d", minor);
 
 	sdev->sector_size = 2048;	/* A guess, just in case */
@@ -882,7 +882,7 @@ static int get_capabilities(struct scsi_cd *cd)
 	    (buffer[n + 6] >> 5) == mechtype_cartridge_changer)
 		cd->cdi.capacity =
 		    cdrom_number_of_slots(&cd->cdi);
-	if (cd->cdi.capacity <= 1)
+	if (cd->cdi.capacity <= CDI_MAX_CAPACITY)
 		/* not a changer */
 		cd->cdi.mask |= CDC_SELECT_DISC;
 	/*else    I don't think it can close its tray
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 67caa909e3e6..51c046354275 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -29,6 +29,8 @@ struct packet_command
 	void			*reserved[1];
 };
 
+#define CDI_MAX_CAPACITY	1
+
 /*
  * _OLD will use PIO transfer on atapi devices, _BPC_* will use DMA
  */