Re: [PATCH] Staging: drivers/gpu/drm/amd/amdgpu: Fix null pointer deference in amdkfd_fence_get_timeline_name

[Christian König]

Am 21.09.24 um 06:25 schrieb Dipendra Khadka:
> On Sat, 21 Sept 2024 at 00:43, Christian König <christian.koenig@xxxxxxx> wrote:
> > Am 20.09.24 um 18:31 schrieb Dipendra Khadka:
> > > On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@xxxxxxx> wrote:
> > > > Am 20.09.24 um 11:09 schrieb Dipendra Khadka:
> > > > > '''
> > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c:108:9: error: Null pointer dereference: fence [nullPointer]
> > > > >     return fence->timeline_name;
> > > > >            ^
> > > > > '''
> > > > >
> > > > > The method to_amdgpu_amdkfd_fence can return NULL incase of empty f
> > > > > or f->ops != &amdkfd_fence_ops.Hence, check has been added .
> > > > > If fence is null , then null is returned.
> > > > Well NAK, completely nonsense. Calling the function with a NULL fence is
> > > > illegal.
> > > Thanks for enlightening me .
> > Well sorry to be so direct, but what the heck did you tried to do here?
> Hi Christian,
>
> cppcheck reported null pointer dereference in the line  " return
> fence->timeline_name;" in the function "static const char
> *amdkfd_fence_get_timeline_name(struct dma_fence *f)".
> In the function , we are getting the value of fence like this :
> "struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);"
>
> When I went through the function " to_amdgpu_amdkfd_fence" whose definition is :
> '''
> struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f)
> {
> struct amdgpu_amdkfd_fence *fence;
>
> if (!f)
> return NULL;
>
> fence = container_of(f, struct amdgpu_amdkfd_fence, base);
> if (f->ops == &amdkfd_fence_ops)
> return fence;
>
> return NULL;
> }
> '''
>
> Here, the function to_amdgpu_amdkfd_fence can return NULL when f is
> empty or f->ops != &amdkfd_fence_ops .So the fence in function
> "amdkfd_fence_get_timeline_name" can be NULL.
> Hence , I thought dereferencing NULL fence like "return
> fence->timeline_name" may result in the runtime crashing. So, I
> proposed this fix. Sorry, I was not aware about the behaviour of the
> fence.
> I am interested in the development and tried to fix this .

Well it's in general a good idea that you looked into this, but you 
should have put more thoughts into it.

That the fence can't be NULL is just implicit when you take a closer 
look at the code.

amdkfd_fence_get_timeline_name() is only called through the pointer in 
amdkfd_fence_ops. This makes the condition "f->ops == &amdkfd_fence_ops" 
always true inside the function.

The only other possibility is that the f parameter is NULL, but that in 
turn is impossible because the function is called like 
f->ops->get_timeline_name(f) and so the caller would have crashed even 
before entering the function.

And finally you didn't looked at the documentation. The kerneldoc for 
get_timeline_name clearly states that the callback is mandatory and 
therefore can't return NULL.

So to sum it up you suggested something which is not only unnecessary, 
but results in documented illegal behavior.

The C language unfortunately doesn't have the necessary annotation 
possibilities that a function can't return a NULL string (at least as 
far as I know). So cppcheck can't know any of this.

Please don't trust the automated tool to much and put a bit more time 
into patches like this.

Regards,
Christian.

> > I mean that is broken on so many different levels that I can't
> > understand why somebody is suggesting something like that.
> >
> > Regards,
> > Christian.
> >
> > > > Regards,
> > > > Christian.
> > > >
> > > > > Signed-off-by: Dipendra Khadka <kdipendra88@xxxxxxxxx>
> > > > > ---
> > > > >     drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 3 +++
> > > > >     1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
> > > > > index 1ef758ac5076..2313babcc944 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c
> > > > > @@ -105,6 +105,9 @@ static const char *amdkfd_fence_get_timeline_name(struct dma_fence *f)
> > > > >     {
> > > > >         struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
> > > > >
> > > > > +     if (!fence)
> > > > > +             return NULL;
> > > > > +
> > > > >         return fence->timeline_name;
> > > > >     }
> > > Regards,
> > > Dipendra Khadka
> Regards,
> Dipendra Khadka