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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0d9c46fd-40a0-4d9b-bd9a-9e0f36da763d@embeddedor.com>
Date:   Thu, 23 Aug 2018 09:43:33 -0500
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Thomas Hellstrom <thellstrom@...are.com>,
        Deepak Singh Rawat <drawat@...are.com>,
        linux-graphics-maintainer <linux-graphics-maintainer@...are.com>,
        Sinclair Yeh <syeh@...are.com>, David Airlie <airlied@...ux.ie>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential
 Spectre v1

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