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

From: Thomas Hellstrom
Date: Tue Aug 21 2018 - 04:19:44 EST

On 08/20/2018 10:53 PM, Deepak Singh Rawat wrote:
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.


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()
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].


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
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;

I must admit my understanding of spectre workings in this case is limited, but why do you need to compare
arg.version against ARRAY_SIZE here, when it is already checked against DRM_VMW_EXECBUF_VERSION earlier?

+ arg.version = array_index_nospec(arg.version,
+ if (copy_from_user(&arg.context_handle,
(void __user *) (data + copy_offset[0]),
copy_offset[arg.version - 1] -
copy_offset[0]) != 0)

Similarly, we want to perform this copy iff arg.version > 1. Why did you remove that check?



Sent to linux-graphics-maintainer@xxxxxxxxxx