* [PATCH] drm/vmwgfx: Fix potential Spectre v1
@ 2018-08-16 19:30 Gustavo A. R. Silva
2018-08-20 20:53 ` [Linux-graphics-maintainer] " Deepak Singh Rawat
0 siblings, 1 reply; 5+ messages in thread
From: Gustavo A. R. Silva @ 2018-08-16 19:30 UTC (permalink / raw)
To: VMware Graphics, Sinclair Yeh, Thomas Hellstrom, David Airlie
Cc: dri-devel, linux-kernel, Gustavo A. R. Silva
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://marc.info/?l=linux-kernel&m=152449131114778&w=2
Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential Spectre v1
2018-08-16 19:30 [PATCH] drm/vmwgfx: Fix potential Spectre v1 Gustavo A. R. Silva
@ 2018-08-20 20:53 ` Deepak Singh Rawat
2018-08-21 8:19 ` Thomas Hellstrom
0 siblings, 1 reply; 5+ messages in thread
From: Deepak Singh Rawat @ 2018-08-20 20:53 UTC (permalink / raw)
To: Gustavo A. R. Silva, linux-graphics-maintainer, Sinclair Yeh,
Thomas Hellstrom, David Airlie
Cc: linux-kernel, dri-devel
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@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> 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@vmware.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential Spectre v1
2018-08-20 20:53 ` [Linux-graphics-maintainer] " Deepak Singh Rawat
@ 2018-08-21 8:19 ` Thomas Hellstrom
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Hellstrom @ 2018-08-21 8:19 UTC (permalink / raw)
To: Deepak Singh Rawat, Gustavo A. R. Silva,
linux-graphics-maintainer, Sinclair Yeh, David Airlie
Cc: linux-kernel, dri-devel
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.
>
> 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]
>>
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>> 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;
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,
>> 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?
Thanks,
Thomas
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Sent to linux-graphics-maintainer@vmware.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential Spectre v1
@ 2018-08-21 8:19 ` Thomas Hellstrom
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Hellstrom @ 2018-08-21 8:19 UTC (permalink / raw)
To: Deepak Singh Rawat, Gustavo A. R. Silva,
linux-graphics-maintainer, Sinclair Yeh, David Airlie
Cc: linux-kernel, dri-devel
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.
>
> 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]
>>
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>> 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;
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,
>> 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?
Thanks,
Thomas
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Sent to linux-graphics-maintainer@vmware.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-graphics-maintainer] [PATCH] drm/vmwgfx: Fix potential Spectre v1
2018-08-21 8:19 ` Thomas Hellstrom
(?)
@ 2018-08-23 14:43 ` Gustavo A. R. Silva
-1 siblings, 0 replies; 5+ messages in thread
From: Gustavo A. R. Silva @ 2018-08-23 14:43 UTC (permalink / raw)
To: Thomas Hellstrom, Deepak Singh Rawat, linux-graphics-maintainer,
Sinclair Yeh, David Airlie
Cc: linux-kernel, dri-devel
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-23 14:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-16 19:30 [PATCH] drm/vmwgfx: Fix potential Spectre v1 Gustavo A. R. Silva
2018-08-20 20:53 ` [Linux-graphics-maintainer] " Deepak Singh Rawat
2018-08-21 8:19 ` Thomas Hellstrom
2018-08-21 8:19 ` Thomas Hellstrom
2018-08-23 14:43 ` Gustavo A. R. Silva
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.