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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Aug 2018 10:19:16 +0200
From:   Thomas Hellstrom <thellstrom@...are.com>
To:     Deepak Singh Rawat <drawat@...are.com>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.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

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@...r.kernel.org
>> Signed-off-by: Gustavo A. R. Silva <gustavo@...eddedor.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@...are.com