RE: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential Spectre v1

[Deepak Singh Rawat]

Looks good to me based on my limited understanding. Thomas/Sinclair can
could you please review and then we can include this in drm-fixes.

Thanks,
Deepak

> 
> arg.version is indirectly controlled by user-space, hence leading to
> a potential exploitation of the Spectre variant 1 vulnerability.
> 
> This issue was detected with the help of Smatch:
> 
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:4526 vmw_execbuf_ioctl()
> warn:
> potential spectre issue 'copy_offset' [w]
> 
> Fix this by sanitizing arg.version before using it to index copy_offset
> 
> Notice that given that speculation windows are large, the policy is
> to kill the speculation on the first load and not worry if it can be
> completed with a dependent load/store [1].
> 
> [1]
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmarc.i
> nfo%2F%3Fl%3Dlinux-
> kernel%26m%3D152449131114778%26w%3D2&data=02%7C01%7Clinux-
> graphics-
> maintainer%40vmware.com%7Cf010b707b8ef4896c1a908d603aebcc6%7Cb39
> 138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636700446365603728&
> sdata=0D8lnUScxOmCCWXLHh8Otc3o%2F1yF1SxgGwIklRdMlXY%3D&re
> served=0
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> index 1f13457..ad91c6e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
> @@ -25,6 +25,7 @@
>   *
> 
> **********************************************************
> ****************/
>  #include <linux/sync_file.h>
> +#include <linux/nospec.h>
> 
>  #include "vmwgfx_drv.h"
>  #include "vmwgfx_reg.h"
> @@ -4520,8 +4521,10 @@ int vmw_execbuf_ioctl(struct drm_device *dev,
> unsigned long data,
>  		return -EINVAL;
>  	}
> 
> -	if (arg.version > 1 &&
> -	    copy_from_user(&arg.context_handle,
> +	if (arg.version >= ARRAY_SIZE(copy_offset))
> +		return -EFAULT;
> +	arg.version = array_index_nospec(arg.version,
> ARRAY_SIZE(copy_offset));
> +	if (copy_from_user(&arg.context_handle,
>  			   (void __user *) (data + copy_offset[0]),
>  			   copy_offset[arg.version - 1] -
>  			   copy_offset[0]) != 0)
> --
> 2.7.4
> 
> _______________________________________________
> Sent to linux-graphics-maintainer@xxxxxxxxxx