[PATCH] staging: greybus: fix possible null-ptr-deref in gb_audio_manager_get_module()

[PATCH] staging: greybus: fix possible null-ptr-deref in gb_audio_manager_get_module()

[Yi Yang]

The gb_audio_manager_get_module() is EXPORT_SYMBOL, and will return NULL
when incoming parameter id < 0, fix possible null-ptr-deref by add check
for return value.

Fixes: 8db00736d365 ("greybus: audio: Add Audio Manager")
Signed-off-by: Yi Yang <yiyang13@huawei.com>
---
 drivers/staging/greybus/audio_manager.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/audio_manager.c b/drivers/staging/greybus/audio_manager.c
index 27ca5f796c5f..1da8804e61ca 100644
--- a/drivers/staging/greybus/audio_manager.c
+++ b/drivers/staging/greybus/audio_manager.c
@@ -111,7 +111,8 @@ struct gb_audio_manager_module *gb_audio_manager_get_module(int id)
 
 	down_read(&modules_rwsem);
 	module = gb_audio_manager_get_locked(id);
-	kobject_get(&module->kobj);
+	if (module)
+		kobject_get(&module->kobj);
 	up_read(&modules_rwsem);
 	return module;
 }
-- 
2.25.1

Re: [PATCH] staging: greybus: fix possible null-ptr-deref in gb_audio_manager_get_module()

[Dan Carpenter]

On Sat, Oct 26, 2024 at 08:11:53AM +0000, Yi Yang wrote:
> The gb_audio_manager_get_module() is EXPORT_SYMBOL, and will return NULL
> when incoming parameter id < 0, fix possible null-ptr-deref by add check
> for return value.
> 
> Fixes: 8db00736d365 ("greybus: audio: Add Audio Manager")
> Signed-off-by: Yi Yang <yiyang13@huawei.com>

Where is gb_audio_manager_get_module() called from?  So far as I can see it's
never used.  Why not just delete it?

regards,
dan carpenter

Re: [PATCH] staging: greybus: fix possible null-ptr-deref in gb_audio_manager_get_module()

[Alex Elder]

On 10/26/24 5:50 AM, Dan Carpenter wrote:
> On Sat, Oct 26, 2024 at 08:11:53AM +0000, Yi Yang wrote:
>> The gb_audio_manager_get_module() is EXPORT_SYMBOL, and will return NULL
>> when incoming parameter id < 0, fix possible null-ptr-deref by add check
>> for return value.
>>
>> Fixes: 8db00736d365 ("greybus: audio: Add Audio Manager")
>> Signed-off-by: Yi Yang <yiyang13@huawei.com>
> 
> Where is gb_audio_manager_get_module() called from?  So far as I can see it's
> never used.  Why not just delete it?
> 
> regards,
> dan carpenter

I agree with this.  I suspected all callers might have guaranteed
that the "id" value passed would be always valid, but... there are
no callers.

It is a simple function, and could be added back again if it is
needed in the future (possibly even by reverting the commit that
removes it).

If you do this, please remove gb_audio_put_module() in the same
patch.  It too has no callers.

Thank you.

					-Alex

Re: [PATCH] staging: greybus: fix possible null-ptr-deref in gb_audio_manager_get_module()

[yiyang (D)]

On 2024/10/28 20:46, Alex Elder wrote:
> On 10/26/24 5:50 AM, Dan Carpenter wrote:
>> On Sat, Oct 26, 2024 at 08:11:53AM +0000, Yi Yang wrote:
>>> The gb_audio_manager_get_module() is EXPORT_SYMBOL, and will return NULL
>>> when incoming parameter id < 0, fix possible null-ptr-deref by add check
>>> for return value.
>>>
>>> Fixes: 8db00736d365 ("greybus: audio: Add Audio Manager")
>>> Signed-off-by: Yi Yang <yiyang13@huawei.com>
>>
>> Where is gb_audio_manager_get_module() called from?  So far as I can 
>> see it's
>> never used.  Why not just delete it?
>>
>> regards,
>> dan carpenter
> 
> I agree with this.  I suspected all callers might have guaranteed
> that the "id" value passed would be always valid, but... there are
> no callers.
> 
> It is a simple function, and could be added back again if it is
> needed in the future (possibly even by reverting the commit that
> removes it).
> 
> If you do this, please remove gb_audio_put_module() in the same
> patch.  It too has no callers.
> 
> Thank you.
> 
>                      -Alex
> 
> .

I tried to find the caller before I modified it, but unfortunately I 
didn't find the caller, so I suspect some non-kernel driver code will 
try to call this functions.

I just found this problem while reading the code and I'm not sure if I 
should remove the function.

regards,
Yiyang
-- 

Re: [PATCH] staging: greybus: fix possible null-ptr-deref in gb_audio_manager_get_module()

[Alex Elder]

On 10/28/24 9:58 PM, yiyang (D) wrote:
> 
> On 2024/10/28 20:46, Alex Elder wrote:
>> On 10/26/24 5:50 AM, Dan Carpenter wrote:
>>> On Sat, Oct 26, 2024 at 08:11:53AM +0000, Yi Yang wrote:
>>>> The gb_audio_manager_get_module() is EXPORT_SYMBOL, and will return 
>>>> NULL
>>>> when incoming parameter id < 0, fix possible null-ptr-deref by add 
>>>> check
>>>> for return value.
>>>>
>>>> Fixes: 8db00736d365 ("greybus: audio: Add Audio Manager")
>>>> Signed-off-by: Yi Yang <yiyang13@huawei.com>
>>>
>>> Where is gb_audio_manager_get_module() called from?  So far as I can 
>>> see it's
>>> never used.  Why not just delete it?
>>>
>>> regards,
>>> dan carpenter
>>
>> I agree with this.  I suspected all callers might have guaranteed
>> that the "id" value passed would be always valid, but... there are
>> no callers.
>>
>> It is a simple function, and could be added back again if it is
>> needed in the future (possibly even by reverting the commit that
>> removes it).
>>
>> If you do this, please remove gb_audio_put_module() in the same
>> patch.  It too has no callers.
>>
>> Thank you.
>>
>>                      -Alex
>>
>> .
> 
> I tried to find the caller before I modified it, but unfortunately I 
> didn't find the caller, so I suspect some non-kernel driver code will 
> try to call this functions.
> 
> I just found this problem while reading the code and I'm not sure if I 
> should remove the function.

The Linux kernel is a unified whole.  This means that if you have the
current version of the kernel source code, it will incorporate *all*
possible callers of a given function (or more generally, references to
or users of a given symbol).  The only possible exception might be
out-of-tree code, but that is not our concern.

If you can't find any callers, none exist.  It's fine to propose
removing the function.  And if the maintainer accepts it, it's a
good change.

					-Alex

> 
> regards,
> Yiyang

Re: [PATCH] staging: greybus: fix possible null-ptr-deref in gb_audio_manager_get_module()

[Dan Carpenter]

On Tue, Oct 29, 2024 at 10:31:14AM -0500, Alex Elder wrote:
> If you can't find any callers, none exist.  It's fine to propose
> removing the function.  And if the maintainer accepts it, it's a
> good change.

Plus in this case specifically Alex is the maintainer so you can already be
pretty sure he'll accept it.  ;)

regards,
dan carpenter