On 2019-07-10 8:58 p.m., Kuehling, Felix wrote:
> 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@xxxxxxx>
>>>>> ---
>>>>> 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.
Leaking page table contents sounds risky to me. There may be other cases
where it's really not needed, but it tends to be hard to predict how
leaked data could be exploited.
> There are other AMDGPU_GEM_CREATE_ flags that don't make sense for user
> mode to choose. This just adds one more.
As a general note not specific to this patch, it would be better if such
flags weren't in the UAPI header (and maybe not named AMDGPU_GEM_*).
--
Earthling Michel Dänzer | https://www.amd.com
Libre software enthusiast | Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
