Messages in this thread | | | Date | Mon, 27 Dec 2021 09:54:10 -0600 | Subject | Re: [PATCH] staging: greybus: audio: Check null pointer | From | 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); >
| |