* Re: [PATCH 1/1] cdrom: Fix spectre-v1 gadget
2023-06-09 13:13 ` [PATCH 1/1] " Jordy Zomer
@ 2023-06-09 17:19 ` kernel test robot
2023-06-09 23:46 ` Pawan Gupta
2023-06-10 19:10 ` Phillip Potter
2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2023-06-09 17:19 UTC (permalink / raw)
To: Jordy Zomer, linux-kernel; +Cc: oe-kbuild-all, phil, Jordy Zomer
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] cdrom: Fix spectre-v1 gadget
2023-06-09 13:13 ` [PATCH 1/1] " Jordy Zomer
2023-06-09 17:19 ` kernel test robot
@ 2023-06-09 23:46 ` Pawan Gupta
2023-06-10 19:10 ` Phillip Potter
2 siblings, 0 replies; 6+ messages in thread
From: Pawan Gupta @ 2023-06-09 23:46 UTC (permalink / raw)
To: Jordy Zomer; +Cc: linux-kernel, phil
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] cdrom: Fix spectre-v1 gadget
2023-06-09 13:13 ` [PATCH 1/1] " Jordy Zomer
2023-06-09 17:19 ` kernel test robot
2023-06-09 23:46 ` Pawan Gupta
@ 2023-06-10 19:10 ` Phillip Potter
2023-06-12 9:35 ` Jordy Zomer
2 siblings, 1 reply; 6+ messages in thread
From: Phillip Potter @ 2023-06-10 19:10 UTC (permalink / raw)
To: Jordy Zomer; +Cc: linux-kernel, jordyzomer, pawan.kumar.gupta, linux-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] cdrom: Fix spectre-v1 gadget
2023-06-10 19:10 ` Phillip Potter
@ 2023-06-12 9:35 ` Jordy Zomer
0 siblings, 0 replies; 6+ messages in thread
From: Jordy Zomer @ 2023-06-12 9:35 UTC (permalink / raw)
To: Phillip Potter; +Cc: linux-kernel, pawan.kumar.gupta, linux-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread