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

[Thomas Hellstrom <thellstrom@vmware.com>]

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