lkml.org 
[lkml]   [2018]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential Spectre v1
Date
Hi all,

On 8/21/18 3:19 AM, Thomas Hellstrom wrote:

>>>   #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?
>
Oh, I wasn't aware of the value in DRM_VMW_EXECBUF_VERSION. But as arg.version is used to index copy_offset,
it is safer to compare its value against the actual size of copy_offset.

So, what do you think if I replace DRM_VMW_EXECBUF_VERSION with ARRAY_SIZE instead of adding a new check
against ARRAY_SIZE?

Something like:

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index 1f13457..3ef9f7b 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"
@@ -4514,11 +4515,12 @@ int vmw_execbuf_ioctl(struct drm_device *dev, unsigned long data,
* arg.version.
*/

- if (unlikely(arg.version > DRM_VMW_EXECBUF_VERSION ||
+ if (unlikely(arg.version > ARRAY_SIZE(copy_offset) ||
arg.version == 0)) {
DRM_ERROR("Incorrect execbuf version.\n");
return -EINVAL;
}
+ arg.version = array_index_nospec(arg.version, ARRAY_SIZE(copy_offset));

if (arg.version > 1 &&
copy_from_user(&arg.context_handle,

>
>
>>> +    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)
>
> Similarly, we want to perform this copy iff arg.version > 1. Why did you remove that check?
>

Yeah, this check must remain in place. I will add it back and send v2.

Thanks for the feedback!
--
Gustavo

\
 
 \ /
  Last update: 2018-08-23 16:45    [W:0.053 / U:0.948 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site