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

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

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