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 <[email protected]>
---
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,
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/[email protected]/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/[email protected]
# 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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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 <[email protected]>
> ---
> 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.
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 <[email protected]>
> ---
> 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
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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