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

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

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

Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com>
---
 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;
 }
 
-- 
2.43.0

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

[Christian König]

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.

Regards,
Christian.

>
> Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com>
> ---
>   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;
>   }
>   

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

[Dipendra Khadka]

On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@amd.com> 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 .

>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Dipendra Khadka <kdipendra88@gmail.com>
> > ---
> >   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

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

[Christian König]

Am 20.09.24 um 18:31 schrieb Dipendra Khadka:
> On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@amd.com> 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?

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

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

[Dipendra Khadka]

On Sat, 21 Sept 2024 at 00:43, Christian König <christian.koenig@amd.com> wrote:
>
> Am 20.09.24 um 18:31 schrieb Dipendra Khadka:
> > On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@amd.com> 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 .

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

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@amd.com> wrote:
>> Am 20.09.24 um 18:31 schrieb Dipendra Khadka:
>>> On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@amd.com> 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@gmail.com>
>>>>> ---
>>>>>     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

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

[Dipendra Khadka]

Hi Christian,

On Mon, 23 Sept 2024 at 18:57, Christian König <christian.koenig@amd.com> wrote:
>
> Am 21.09.24 um 06:25 schrieb Dipendra Khadka:
> > On Sat, 21 Sept 2024 at 00:43, Christian König <christian.koenig@amd.com> wrote:
> >> Am 20.09.24 um 18:31 schrieb Dipendra Khadka:
> >>> On Fri, 20 Sept 2024 at 16:01, Christian König <christian.koenig@amd.com> 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.
>

I am learning driver development and was not sure how it works. Now, I got it.

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

Thank you so much for the insight and your time. I will make sure to
see the kernel doc as well as try to think more.

Best Regards,
Dipendra

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