2023-04-17 14:40:06

by Nikita Zhandarovich

Subject: [PATCH] drm/ttm: fix null-ptr-deref in radeon_ttm_tt_populate()

Currently, drm_prime_sg_to_page_addr_arrays() dereferences 'gtt->ttm'
without ensuring that 'gtt' (and therefore 'gtt->tmm') is not NULL.

Fix this by testing 'gtt' for NULL value before dereferencing.

Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.

Fixes: 40f5cf996991 ("drm/radeon: add PRIME support (v2)")
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 1e8e287e113c..33d01c3bdee4 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -553,7 +553,7 @@ static int radeon_ttm_tt_populate(struct ttm_device *bdev,
return 0;
}

- if (slave && ttm->sg) {
+ if (gtt && slave && ttm->sg) {
drm_prime_sg_to_dma_addr_array(ttm->sg, gtt->ttm.dma_address,
ttm->num_pages);
return 0;
--
2.25.1

2023-04-17 14:43:43

by Christian König

Subject: Re: [PATCH] drm/ttm: fix null-ptr-deref in radeon_ttm_tt_populate()

Am 17.04.23 um 16:34 schrieb Nikita Zhandarovich:
> Currently, drm_prime_sg_to_page_addr_arrays() dereferences 'gtt->ttm'
> without ensuring that 'gtt' (and therefore 'gtt->tmm') is not NULL.
>
> Fix this by testing 'gtt' for NULL value before dereferencing.
>
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
>
> Fixes: 40f5cf996991 ("drm/radeon: add PRIME support (v2)")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 1e8e287e113c..33d01c3bdee4 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -553,7 +553,7 @@ static int radeon_ttm_tt_populate(struct ttm_device *bdev,
> return 0;
> }
>
> - if (slave && ttm->sg) {
> + if (gtt && slave && ttm->sg) {

The gtt variable is derived from the ttm variable and so never NULL
here. The only case when this can be NULL is for AGP and IIRC we don't
support DMA-buf in this case.

> drm_prime_sg_to_dma_addr_array(ttm->sg, gtt->ttm.dma_address,

Just use ttm->dma_addresses instead of gtt->ttm.dma_address here to make
your automated checker happy.

Regards,
Christian.

> ttm->num_pages);
> return 0;

2023-04-17 14:59:19

by Nikita Zhandarovich

Subject: Re: [PATCH] drm/ttm: fix null-ptr-deref in radeon_ttm_tt_populate()

On 4/17/23 07:42, Christian König wrote:
>
>
> Am 17.04.23 um 16:34 schrieb Nikita Zhandarovich:
>> Currently, drm_prime_sg_to_page_addr_arrays() dereferences 'gtt->ttm'
>> without ensuring that 'gtt' (and therefore 'gtt->tmm') is not NULL.
>>
>> Fix this by testing 'gtt' for NULL value before dereferencing.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 40f5cf996991 ("drm/radeon: add PRIME support (v2)")
>> Signed-off-by: Nikita Zhandarovich <[email protected]>
>> ---
>>   drivers/gpu/drm/radeon/radeon_ttm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
>> b/drivers/gpu/drm/radeon/radeon_ttm.c
>> index 1e8e287e113c..33d01c3bdee4 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
>> @@ -553,7 +553,7 @@ static int radeon_ttm_tt_populate(struct
>> ttm_device *bdev,
>>           return 0;
>>       }
>>   -    if (slave && ttm->sg) {
>> +    if (gtt && slave && ttm->sg) {
>
> The gtt variable is derived from the ttm variable and so never NULL
> here. The only case when this can be NULL is for AGP and IIRC we don't
> support DMA-buf in this case.
>
>>           drm_prime_sg_to_dma_addr_array(ttm->sg, gtt->ttm.dma_address,
>
> Just use ttm->dma_addresses instead of gtt->ttm.dma_address here to make
> your automated checker happy.
>
> Regards,
> Christian.
>
>>                              ttm->num_pages);
>>           return 0;
>

Thank you for your reply, you are absolutely right.
Apologies for wasting your time.

Nikita