[PATCH 2/3] drm/amdgpu: Mark KFD VRAM allocations as sensitive

[PATCH 2/3] drm/amdgpu: Mark KFD VRAM allocations as sensitive

[Kuehling, Felix]

Memory used by KFD applications can contain sensitive information that
should not be leaked to other processes. The current approach to prevent
leaks is to clear VRAM at allocation time. This is not effective because
memory can be reused in other ways without being cleared. Synchronously
clearing memory on the allocation path also carries a significant
performance penalty.

Stop clearing memory at allocation time. Instead mark the memory as
sensitive to indicate that it needs to be cleared before it is freed.

Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index f5ecf28eb37c..aca1513eb3c3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1089,7 +1089,7 @@ int amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu(
 	 */
 	if (flags & ALLOC_MEM_FLAGS_VRAM) {
 		domain = alloc_domain = AMDGPU_GEM_DOMAIN_VRAM;
-		alloc_flags = AMDGPU_GEM_CREATE_VRAM_CLEARED;
+		alloc_flags = AMDGPU_GEM_CREATE_VRAM_SENSITIVE;
 		alloc_flags |= (flags & ALLOC_MEM_FLAGS_PUBLIC) ?
 			AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED :
 			AMDGPU_GEM_CREATE_NO_CPU_ACCESS;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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@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.


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Alex Deucher]

On Tue, Jul 9, 2019 at 1:32 AM Kuehling, Felix <Felix.Kuehling@amd.com> 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@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)

If we decide to go this route, I'd like to make this flag more explicit.  E.g.,

AMDGPU_GEM_CREATE_VRAM_CLEAR_ON_FREE

Alex

>
>  struct drm_amdgpu_gem_create_in  {
>         /** the requested memory size */
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Kuehling, Felix]

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@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. It's more a convenience feature. Therefore I think it 
still has a place as that.

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.

Regards,
   Felix


>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Kuehling, Felix]

On 2019-07-09 8:59 a.m., Alex Deucher wrote:
> On Tue, Jul 9, 2019 at 1:32 AM Kuehling, Felix <Felix.Kuehling@amd.com> 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@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)
> If we decide to go this route, I'd like to make this flag more explicit.  E.g.,
>
> AMDGPU_GEM_CREATE_VRAM_CLEAR_ON_FREE

It's more than just clear on free. Memory needs to get cleared whenever 
the backing physical memory gets released to be reused by other 
processes. That can be because of a free, or because the buffer moved, 
or because it was evicted.

Regards,
   Felix


>
> Alex
>
>>   struct drm_amdgpu_gem_create_in  {
>>          /** the requested memory size */
>> --
>> 2.17.1
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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@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.

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).


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@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).
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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@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.

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx