[PATCH] drm/vmwgfx: Fix potential Spectre v1

[PATCH] drm/vmwgfx: Fix potential Spectre v1

[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

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

[Deepak Singh Rawat]

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

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

[Thomas Hellstrom]

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

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

[Thomas Hellstrom]

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

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

[Gustavo A. R. Silva]

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