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

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

[Jordy Zomer]

This patch fixes a spectre-v1 gadget in cdrom.
The gadget could be triggered by,
speculatviely bypassing the cdi->capacity check.

Jordy Zomer (1):
  cdrom: Fix spectre-v1 gadget

 drivers/cdrom/cdrom.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.41.0.162.gfafddb0af9-goog

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

[Jordy Zomer]

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 416f723a2dbb..3c349bc0a269 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -233,6 +233,7 @@
 
 -------------------------------------------------------------------------*/
 
+#include "asm/barrier.h"
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #define REVISION "Revision: 3.20"
@@ -2329,6 +2330,8 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
 	if (arg >= cdi->capacity)
 		return -EINVAL;
 
+	arg = array_index_mask_nospec(arg, cdi->capacity);
+
 	info = kmalloc(sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
-- 
2.41.0.162.gfafddb0af9-goog

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

[kernel test robot]

Hi Jordy,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.4-rc5 next-20230609]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jordy-Zomer/cdrom-Fix-spectre-v1-gadget/20230609-211545
base:   linus/master
patch link:    https://lore.kernel.org/r/20230609131355.71130-2-jordyzomer%40google.com
patch subject: [PATCH 1/1] cdrom: Fix spectre-v1 gadget
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230610/202306100143.yzdJpUid-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git checkout linus/master
        b4 shazam https://lore.kernel.org/r/20230609131355.71130-2-jordyzomer@google.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash drivers/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306100143.yzdJpUid-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/cdrom/cdrom.c: In function 'cdrom_ioctl_media_changed':
>> drivers/cdrom/cdrom.c:2333:15: error: implicit declaration of function 'array_index_mask_nospec' [-Werror=implicit-function-declaration]
    2333 |         arg = array_index_mask_nospec(arg, cdi->capacity);
         |               ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/array_index_mask_nospec +2333 drivers/cdrom/cdrom.c

  2314	
  2315	static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
  2316			unsigned long arg)
  2317	{
  2318		struct cdrom_changer_info *info;
  2319		int ret;
  2320	
  2321		cd_dbg(CD_DO_IOCTL, "entering CDROM_MEDIA_CHANGED\n");
  2322	
  2323		if (!CDROM_CAN(CDC_MEDIA_CHANGED))
  2324			return -ENOSYS;
  2325	
  2326		/* cannot select disc or select current disc */
  2327		if (!CDROM_CAN(CDC_SELECT_DISC) || arg == CDSL_CURRENT)
  2328			return media_changed(cdi, 1);
  2329	
  2330		if (arg >= cdi->capacity)
  2331			return -EINVAL;
  2332	
> 2333		arg = array_index_mask_nospec(arg, cdi->capacity);
  2334	
  2335		info = kmalloc(sizeof(*info), GFP_KERNEL);
  2336		if (!info)
  2337			return -ENOMEM;
  2338	
  2339		ret = cdrom_read_mech_status(cdi, info);
  2340		if (!ret)
  2341			ret = info->slots[arg].change;
  2342		kfree(info);
  2343		return ret;
  2344	}
  2345	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Pawan Gupta]

On Fri, Jun 09, 2023 at 01:13:55PM +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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 416f723a2dbb..3c349bc0a269 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -233,6 +233,7 @@
>  
>  -------------------------------------------------------------------------*/
>  
> +#include "asm/barrier.h"
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #define REVISION "Revision: 3.20"
> @@ -2329,6 +2330,8 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
>  	if (arg >= cdi->capacity)
>  		return -EINVAL;
>  
> +	arg = array_index_mask_nospec(arg, cdi->capacity);

array_index_nospec() is the correct function to use. The above generates
a mask and not the original value. Also it is effective when the second
argument is a build time constant. If thats not possible and this
function is not called very often barrier_nospec() is also an option.

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

[Phillip Potter]

On Fri, Jun 09, 2023 at 01:13:55PM +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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 416f723a2dbb..3c349bc0a269 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -233,6 +233,7 @@
>  
>  -------------------------------------------------------------------------*/
>  
> +#include "asm/barrier.h"
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #define REVISION "Revision: 3.20"
> @@ -2329,6 +2330,8 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
>  	if (arg >= cdi->capacity)
>  		return -EINVAL;
>  
> +	arg = array_index_mask_nospec(arg, cdi->capacity);
> +
>  	info = kmalloc(sizeof(*info), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
> -- 
> 2.41.0.162.gfafddb0af9-goog
> 

Hi Jordy,

Thanks for the patch, much appreciated. Sadly, as Pawan has already
pointed out, array_index_mask_nospec actually changes the behaviour of
this function, such that 'arg' would no longer be an array index.

In addition, it seems to have triggered the kernel test robot with an
alpha build error.

Regards,
Phil

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

[Jordy Zomer]

Thanks both! I assumed array_index_mask_nospec was the same as
array_index_nospec. I'll send a V2 your way soon :)


On Sat, Jun 10, 2023 at 9:10 PM Phillip Potter <phil@philpotter.co.uk> wrote:
>
> On Fri, Jun 09, 2023 at 01:13:55PM +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 | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> > index 416f723a2dbb..3c349bc0a269 100644
> > --- a/drivers/cdrom/cdrom.c
> > +++ b/drivers/cdrom/cdrom.c
> > @@ -233,6 +233,7 @@
> >
> >  -------------------------------------------------------------------------*/
> >
> > +#include "asm/barrier.h"
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> >  #define REVISION "Revision: 3.20"
> > @@ -2329,6 +2330,8 @@ static int cdrom_ioctl_media_changed(struct cdrom_device_info *cdi,
> >       if (arg >= cdi->capacity)
> >               return -EINVAL;
> >
> > +     arg = array_index_mask_nospec(arg, cdi->capacity);
> > +
> >       info = kmalloc(sizeof(*info), GFP_KERNEL);
> >       if (!info)
> >               return -ENOMEM;
> > --
> > 2.41.0.162.gfafddb0af9-goog
> >
>
> Hi Jordy,
>
> Thanks for the patch, much appreciated. Sadly, as Pawan has already
> pointed out, array_index_mask_nospec actually changes the behaviour of
> this function, such that 'arg' would no longer be an array index.
>
> In addition, it seems to have triggered the kernel test robot with an
> alpha build error.
>
> Regards,
> Phil