[PATCH] staging: greybus: audio: Check null pointer

[PATCH] staging: greybus: audio: Check null pointer

[Jiasheng Jiang]

As the possible alloc failure of devm_kcalloc, it could return null
pointer.
To prevent the dereference of the null pointer, it should be checked.

Fixes: e65579e335da ("greybus: audio: topology: Enable enumerated control support")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/staging/greybus/audio_topology.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
index 1fc7727ab7be..e9f47a1f0d28 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -146,7 +146,11 @@ static const char **gb_generate_enum_strings(struct gbaudio_module_info *gb,
 	__u8 *data;
 
 	items = le32_to_cpu(gbenum->items);
+
 	strings = devm_kcalloc(gb->dev, items, sizeof(char *), GFP_KERNEL);
+	if (!strings)
+		return NULL;
+
 	data = gbenum->names;
 
 	for (i = 0; i < items; i++) {
@@ -654,7 +658,10 @@ static int gbaudio_tplg_create_enum_kctl(struct gbaudio_module_info *gb,
 
 	/* since count=1, and reg is dummy */
 	gbe->items = le32_to_cpu(gb_enum->items);
+
 	gbe->texts = gb_generate_enum_strings(gb, gb_enum);
+	if (!gbe->texts)
+		return -ENOMEM;
 
 	/* debug enum info */
 	dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items,
@@ -861,7 +868,10 @@ static int gbaudio_tplg_create_enum_ctl(struct gbaudio_module_info *gb,
 
 	/* since count=1, and reg is dummy */
 	gbe->items = le32_to_cpu(gb_enum->items);
+
 	gbe->texts = gb_generate_enum_strings(gb, gb_enum);
+	if (!gbe->texts)
+		return -ENOMEM;
 
 	/* debug enum info */
 	dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items,
@@ -1032,8 +1042,12 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
 			csize += offsetof(struct gb_audio_ctl_elem_info, value);
 			csize += offsetof(struct gb_audio_enumerated, names);
 			csize += le16_to_cpu(gbenum->names_length);
+
 			control->texts = (const char * const *)
 				gb_generate_enum_strings(module, gbenum);
+			if (!control->texts)
+				return -ENOMEM;
+
 			control->items = le32_to_cpu(gbenum->items);
 		} else {
 			csize = sizeof(struct gb_audio_control);
@@ -1181,8 +1195,12 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module,
 			csize += offsetof(struct gb_audio_ctl_elem_info, value);
 			csize += offsetof(struct gb_audio_enumerated, names);
 			csize += le16_to_cpu(gbenum->names_length);
+
 			control->texts = (const char * const *)
 				gb_generate_enum_strings(module, gbenum);
+			if (!control->texts)
+				return -ENOMEM;
+
 			control->items = le32_to_cpu(gbenum->items);
 		} else {
 			csize = sizeof(struct gb_audio_control);
-- 
2.25.1

Re: [PATCH] staging: greybus: audio: Check null pointer

[Alex Elder]

On 12/24/21 3:03 AM, Jiasheng Jiang wrote:
> As the possible alloc failure of devm_kcalloc, it could return null
> pointer.
> To prevent the dereference of the null pointer, it should be checked.

I think this is a good change, but I would like you to improve
the description, and fix some different bugs introduced by your
change.

What you are specifically doing is checking for a null return
from devm_kcalloc() in gb_generate_enum_strings(), and are
returning the NULL pointer if that occurs.  That means you
need to update all the callers of gb_generate_enum_strings()
to also handle a possible null return value.

The fix does a good thing, and your description is correct
about what you are fixing.  But it should supply more
complete context for the change.

More below.

> Fixes: e65579e335da ("greybus: audio: topology: Enable enumerated control support")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>   drivers/staging/greybus/audio_topology.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 1fc7727ab7be..e9f47a1f0d28 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -146,7 +146,11 @@ static const char **gb_generate_enum_strings(struct gbaudio_module_info *gb,
>   	__u8 *data;
>   
>   	items = le32_to_cpu(gbenum->items);
> +
>   	strings = devm_kcalloc(gb->dev, items, sizeof(char *), GFP_KERNEL);
> +	if (!strings)
> +		return NULL;
> +
>   	data = gbenum->names;
>   
>   	for (i = 0; i < items; i++) {
> @@ -654,7 +658,10 @@ static int gbaudio_tplg_create_enum_kctl(struct gbaudio_module_info *gb,
>   
>   	/* since count=1, and reg is dummy */
>   	gbe->items = le32_to_cpu(gb_enum->items);
> +
>   	gbe->texts = gb_generate_enum_strings(gb, gb_enum);
> +	if (!gbe->texts)
> +		return -ENOMEM;
>   
>   	/* debug enum info */
>   	dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items,
> @@ -861,7 +868,10 @@ static int gbaudio_tplg_create_enum_ctl(struct gbaudio_module_info *gb,
>   
>   	/* since count=1, and reg is dummy */
>   	gbe->items = le32_to_cpu(gb_enum->items);
> +
>   	gbe->texts = gb_generate_enum_strings(gb, gb_enum);
> +	if (!gbe->texts)
> +		return -ENOMEM;
>   
>   	/* debug enum info */
>   	dev_dbg(gb->dev, "Max:%d, name_length:%d\n", gbe->items,
> @@ -1032,8 +1042,12 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
>   			csize += offsetof(struct gb_audio_ctl_elem_info, value);
>   			csize += offsetof(struct gb_audio_enumerated, names);
>   			csize += le16_to_cpu(gbenum->names_length);
> +
>   			control->texts = (const char * const *)
>   				gb_generate_enum_strings(module, gbenum);
> +			if (!control->texts)
> +				return -ENOMEM;
> +

You can't simply return here.  If you look a bit above this,
where the call to allocate a control structure is done, you
see that a NULL return there jumps to the "error" label, so
any already allocated and initialized control widgets get
cleaned up before returning.

>   			control->items = le32_to_cpu(gbenum->items);
>   		} else {
>   			csize = sizeof(struct gb_audio_control);
> @@ -1181,8 +1195,12 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module,
>   			csize += offsetof(struct gb_audio_ctl_elem_info, value);
>   			csize += offsetof(struct gb_audio_enumerated, names);
>   			csize += le16_to_cpu(gbenum->names_length);
> +
>   			control->texts = (const char * const *)
>   				gb_generate_enum_strings(module, gbenum);
> +			if (!control->texts)
> +				return -ENOMEM;
> +

You have basically the same issue here.  You can't just return,
you must do some cleanup too.

					-Alex

>   			control->items = le32_to_cpu(gbenum->items);
>   		} else {
>   			csize = sizeof(struct gb_audio_control);
> 

Re: Re: [PATCH] staging: greybus: audio: Check null pointer

[Jiasheng Jiang]

Sorry the previous email is forgetten to wrap line.
This email is corrected and the content is the same.

On Mon, Dec 27, 2021 at 11:54:10PM +0800, Alex Elder wrote:
> I think this is a good change, but I would like you to improve
> the description, and fix some different bugs introduced by your
> change.
>
> What you are specifically doing is checking for a null return
> from devm_kcalloc() in gb_generate_enum_strings(), and are
> returning the NULL pointer if that occurs.  That means you
> need to update all the callers of gb_generate_enum_strings()
> to also handle a possible null return value.
>
> The fix does a good thing, and your description is correct
> about what you are fixing.  But it should supply more
> complete context for the change.

Thanks for your advice, I will correct my description in next version.
But I still have some question about the devm_kzalloc().

> You can't simply return here.  If you look a bit above this,
> where the call to allocate a control structure is done, you
> see that a NULL return there jumps to the "error" label, so
> any already allocated and initialized control widgets get
> cleaned up before returning.

Actually, I have already thought of whether it needs to free after the
devm_kzalloc().
As we can find in the gbaudio_tplg_create_widget(), the widget_kctls is
allocated by devm_kzalloc(), but isn't released when
gbaudio_tplg_create_wcontrol() fails and goto error.
And I check of the comment of the devm_kmalloc() in `drivers/base/devres.c`,
because devm_kzalloc() returns devm_kmalloc().
And it says that "Memory allocated with this function is automatically
freed on driver detach."
So there is no need to free the memory manually.
Is that right?
And I am sorry again because of the lack of the above explanation in my
commit message.
I will also add to my new commit.

Thanks,
Jiang

Re: [PATCH] staging: greybus: audio: Check null pointer

[Alex Elder]

On 12/27/21 8:02 PM, Jiasheng Jiang wrote:
> Sorry the previous email is forgetten to wrap line.
> This email is corrected and the content is the same.
> 
> On Mon, Dec 27, 2021 at 11:54:10PM +0800, Alex Elder wrote:
>> I think this is a good change, but I would like you to improve
>> the description, and fix some different bugs introduced by your
>> change.
>>
>> What you are specifically doing is checking for a null return
>> from devm_kcalloc() in gb_generate_enum_strings(), and are
>> returning the NULL pointer if that occurs.  That means you
>> need to update all the callers of gb_generate_enum_strings()
>> to also handle a possible null return value.
>>
>> The fix does a good thing, and your description is correct
>> about what you are fixing.  But it should supply more
>> complete context for the change.
> 
> Thanks for your advice, I will correct my description in next version.
> But I still have some question about the devm_kzalloc().
> 
>> You can't simply return here.  If you look a bit above this,
>> where the call to allocate a control structure is done, you
>> see that a NULL return there jumps to the "error" label, so
>> any already allocated and initialized control widgets get
>> cleaned up before returning.
> 
> Actually, I have already thought of whether it needs to free after the
> devm_kzalloc().
> As we can find in the gbaudio_tplg_create_widget(), the widget_kctls is
> allocated by devm_kzalloc(), but isn't released when
> gbaudio_tplg_create_wcontrol() fails and goto error.
> And I check of the comment of the devm_kmalloc() in `drivers/base/devres.c`,
> because devm_kzalloc() returns devm_kmalloc().
> And it says that "Memory allocated with this function is automatically
> freed on driver detach."
> So there is no need to free the memory manually.
> Is that right?

You are partially right, but you're missing something.  Yes, anything
allocated with devm_kzalloc() will be freed automatically when the
last reference to the device is dropped.

But the two places you're returning are in the middle of a loop (in 
gbaudio_tplg_create_widget() and gbaudio_tplg_process_kcontrols()).
Each is building up a sort of hierarchical data structure, and as
you can see, the existing code takes care to de-construct the partially
built structure in the event an error occurs before it completes.

There is a chance that your simple return would "work", but by
reading the surrounding code you should recognize that the proper
way to handle the error is to jump to the cleanup at the end.

The other alternative would be to fix those functions so they
don't do that controlled cleanup on error and simply return
early (as you were proposing).  But without digging deeper, I
would assume the original developers designed it this way
very intentionally, because it avoids a problem somewhere.

					-Alex

> And I am sorry again because of the lack of the above explanation in my
> commit message.
> I will also add to my new commit.
> 
> Thanks,
> Jiang
> 

Re: [PATCH] staging: greybus: audio: Check null pointer

[Dan Carpenter]

On Tue, Dec 28, 2021 at 08:50:22AM -0600, Alex Elder wrote:
> 
> There is a chance that your simple return would "work"

No.  Your original comment was correct.  It has to "ret = -ENOMEM;
goto error;"

regards,
dan carpenter