[PATCH 1/3] drm/amdgpu: Add flag for allocating memory for sensitive data

[Kuehling, Felix]

On 2019-07-10 3:22 a.m., Michel Dänzer wrote:
> On 2019-07-09 9:00 p.m., Kuehling, Felix wrote:
>> On 2019-07-09 6:34 a.m., Michel Dänzer wrote:
>>> On 2019-07-09 7:32 a.m., Kuehling, Felix wrote:
>>>> This memory allocation flag will be used to indicate BOs containing
>>>> sensitive data that should not be leaked to other processes.
>>>>
>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
>>>> ---
>>>>    include/uapi/drm/amdgpu_drm.h | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>>> index 61870478bc9c..58659c28c26e 100644
>>>> --- a/include/uapi/drm/amdgpu_drm.h
>>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>>> @@ -131,6 +131,10 @@ extern "C" {
>>>>     * for the second page onward should be set to NC.
>>>>     */
>>>>    #define AMDGPU_GEM_CREATE_MQD_GFX9		(1 << 8)
>>>> +/* Flag that BO may contain sensitive data that must be cleared before
>>>> + * releasing the memory
>>>> + */
>>>> +#define AMDGPU_GEM_CREATE_VRAM_SENSITIVE	(1 << 9)
>>>>    
>>>>    struct drm_amdgpu_gem_create_in  {
>>>>    	/** the requested memory size */
>>>>
>>> This flag essentially means "Please don't leak my BO contents".
>>> Similarly, AMDGPU_GEM_CREATE_VRAM_CLEARED essentially means "Please
>>> don't let me see previous memory contents".
>>>
>>> I'd argue that neither flag should really be needed; BO contents
>>> shouldn't be leaked by default.
>> My conclusion from previous discussions was that CREATE_VRAM_CLEARED has
>> no security implications. It's basically completely ineffective as a
>> security measure.
> Absolutely, which is why I argued against it when it was proposed.
>
>> It's more a convenience feature. Therefore I think it still has a place
>> as that.
> It'd be a no-op if memory was always cleared. :)
>
>
>> I'd agree on principle that data shouldn't be leaked by default, but it
>> has been the default for a long time. My impression was that graphics
>> guys cared more about performance than security. So changing the default
>> may be a hard sell. On the compute side we already took a big
>> performance hit by clearing all our VRAM, so this change would be an
>> improvement for us. Therefore I think it still makes sense to let the
>> application choose.
> What exactly could userspace be allowed to choose though? I can only
> think of disabling the clearing of memory it allocates ("Please leak my
> BO contents"), which seems of rather dubious value.

I'm not insisting to leave it to user mode. But I think it makes sense 
to have the choice per BO. The GEM ioctl could set the flag by default 
for all user mode allocations. But some kernel mode BOs may not need it. 
E.g. firmware, page tables, etc.

There are other AMDGPU_GEM_CREATE_ flags that don't make sense for user 
mode to choose. This just adds one more.

Regards,
   Felix


>
> What might be feasible is allowing the system administrator to disable
> it, similar to the mitigations for Meltdown/Spectre/... vulnerabilities.
> OTOH I don't know of any mechanism for disabling the clearing of normal
> system RAM (which is also effective at least to some degree for BOs
> outside of VRAM, making the whole thing a bit inconsistent).
>
>