Potential NULL pointer dereference in radeon_ttm_tt_populate

[Koenig, Christian]

Am 19.03.19 um 21:01 schrieb Shaobo He:
> > See here:
> > #if IS_ENABLED(CONFIG_AGP)
> >          if (rdev->flags & RADEON_IS_AGP) {
> >                  return ttm_agp_tt_populate(ttm, ctx);
> >          }
> > #endif
> >
>
> This code appears to be after the potential location of NULL pointer 
> dereference that I pointed out. Please see,
>
> ```
>     if (slave && ttm->sg) {
>         drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
>                        gtt->ttm.dma_address,ttm->num_pages);
>         ttm->state = tt_unbound;
>         return 0;
>     }
> ```
> The argument expression `gtt->ttm.dma_address` is the potentially 
> offending code if `gtt` is NULL.

Yeah, but same thing for this as well.

That code path is only taken for DMA_buf imports, which is something not 
available for AGP either.

So that NULL pointer deref can't trigger either,
Christian.

>
> Shaobo
> On 3/19/19 12:28 PM, Christian König wrote:
>>> ... or the backend methods is not `radeon_backend_func`.
>> That's the case when it is an AGP backend.
>>
>>> Moreover, could you point out the check of such case before the 
>>> offending code?
>>
>> See here:
>> #if IS_ENABLED(CONFIG_AGP)
>>          if (rdev->flags & RADEON_IS_AGP) {
>>                  return ttm_agp_tt_populate(ttm, ctx);
>>          }
>> #endif
>>
>>
>>> Meaning the check of whether `ttm` is an AGP ttm?
>> Well exactly that's the point, we never check that the ttm structure 
>> is an AGP ttm.
>>
>> We check if the device is an AGP device and if that's the case then 
>> the ttm structure must be an AGP structure as well.
>>
>> Regards,
>> Christian.
>>
>>
>> Am 19.03.19 um 17:46 schrieb Shaobo He:
>>> Hi Christian,
>>>
>>> Thank you very much for your reply. I'm a little confused here so I 
>>> really appreciate if you could clarify it more.
>>>
>>> For example, I don't understand why the condition of function 
>>> `radeon_ttm_tt_to_gtt` returning NULL is the argument being an AGP 
>>> ttm. Based on its definition, it returns NULL when the argument is 
>>> NULL or the backend methods is not `radeon_backend_func`. Is there 
>>> any correlation that I missed here?
>>>
>>> Moreover, could you point out the check of such case before the 
>>> offending code? Meaning the check of whether `ttm` is an AGP ttm?
>>>
>>> Best,
>>> Shaobo
>>> On 2019/3/19 3:16, Christian König wrote:
>>>> Hi Shaobo,
>>>>
>>>> that question came up a couple of times now. And the answer is: No, 
>>>> there can't be a NULL pointer dereference.
>>>>
>>>> The function radeon_ttm_tt_to_gtt returns NULL only when it is an 
>>>> AGP ttm structure, and that case is checked right before the 
>>>> offending code.
>>>>
>>>> Unfortunately I don't see how an automated code checker should ever 
>>>> be able to figure that out by itself.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 18.03.19 um 21:58 schrieb Shaobo He:
>>>>> Hello everyone,
>>>>>
>>>>> My name is Shaobo He and I am a graduate student at University of 
>>>>> Utah. I am using a static analysis tool to search for null pointer 
>>>>> dereferences and came across a potentially invalid memory access 
>>>>> in the file drivers/gpu/drm/radeon/radeon_ttm.c: in function 
>>>>> `radeon_ttm_tt_populate`, function `radeon_ttm_tt_to_gtt` can 
>>>>> return a NULL pointer which is dereferenced by the call to 
>>>>> `drm_prime_sg_to_page_addr_arrays`.
>>>>>
>>>>> Please let me know if it makes sense. I am looking forward to your 
>>>>> reply.
>>>>>
>>>>> Best,
>>>>> Shaobo
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx