| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c81067a4-e8d7-6514-079d-283f14d6c5ba@ieee.org>
Date: Mon, 3 Jan 2022 09:10:05 -0600
From: Alex Elder <elder@...e.org>
To: Jiasheng Jiang <jiasheng@...as.ac.cn>, vaibhav.sr@...il.com,
mgreer@...malcreek.com, johan@...nel.org, elder@...nel.org,
gregkh@...uxfoundation.org
Cc: greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: greybus: audio: Check null pointer
On 12/28/21 8:28 PM, Jiasheng Jiang wrote:
> On Tue, Dec 28, 2021 at 10:50:22PM +0800, Alex Elder wrote:
>> 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.
>
> Yes, I agree with your opinion and this time I use "goto error" instead
> of directly return in the two callers.
> The new patch is as follow.
>
> As the possible alloc failure of devm_kcalloc(), it could return null
> pointer.
> Therefore, 'strings' should be checked and return NULL if alloc fails to
> prevent the dereference of the NULL pointer.
> Also, the caller should also deal with the return value of the
> gb_generate_enum_strings() and return -ENOMEM if returns NULL.
> Moreover, because the memory allocated with devm_kzalloc() will be
> freed automatically when the last reference to the device is dropped,
> the 'gbe' in gbaudio_tplg_create_enum_kctl() and
> gbaudio_tplg_create_enum_ctl() do not need to free manually.
> But the 'control' in gbaudio_tplg_create_widget() and
> gbaudio_tplg_process_kcontrols() has a specially error handle to
> cleanup.
> So it should be better to cleanup 'control' when fails.
I haven't really looked at this yet, but in any case I would like
you to send version 3 of this patch.
The reason is that you should not include the message above in
the patch itself.
To be clear, I really appreciate your response; please *do* respond
to review comments. But do so in e-mail only, and then follow up
with a new patch, using the same basic subject (with a new version)
and (roughly) the same description.
So your patch should have a subject line with v3. It should then
contain the original description (not indented with ">>" or anything),
updated or improved if appropriate to reflect the current state of
the patch. Then, below the "---" line you should include a patch
history, with a line or two describing each version. For example,
it might look something like:
---
v3: Same code as v2, but fixed description as requested.
v2: Updated to use common error processing at the end of both
gbaudio_tplg_create_widget() and gbaudio_tplg_process_kcontrols().
I will review version 3, and if I don't see anything else wrong with
it I'll offer a "Signed-off-by" tag.
-Alex
> Fixes: e65579e335da ("greybus: audio: topology: Enable enumerated control support")
> Signed-off-by: Jiasheng Jiang <jiasheng@...as.ac.cn>
> ---
> Changelog:
>
> v1 -> v2
>
> *Change 1. Add the cleanup process when alloc fails in two callers and
> refine the commit message.
> ---
> drivers/staging/greybus/audio_topology.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
> index 1fc7727ab7be..6bba735d2e5c 100644
> --- a/drivers/staging/greybus/audio_topology.c
> +++ b/drivers/staging/greybus/audio_topology.c
> @@ -147,6 +147,9 @@ static const char **gb_generate_enum_strings(struct gbaudio_module_info *gb,
>
> 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++) {
> @@ -655,6 +658,8 @@ 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,
> @@ -862,6 +867,8 @@ 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,
> @@ -1034,6 +1041,10 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module,
> csize += le16_to_cpu(gbenum->names_length);
> control->texts = (const char * const *)
> gb_generate_enum_strings(module, gbenum);
> + if (!control->texts) {
> + ret = -ENOMEM;
> + goto error;
> + }
> control->items = le32_to_cpu(gbenum->items);
> } else {
> csize = sizeof(struct gb_audio_control);
> @@ -1183,6 +1194,10 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module,
> csize += le16_to_cpu(gbenum->names_length);
> control->texts = (const char * const *)
> gb_generate_enum_strings(module, gbenum);
> + if (!control->texts) {
> + ret = -ENOMEM;
> + goto error;
> + }
> control->items = le32_to_cpu(gbenum->items);
> } else {
> csize = sizeof(struct gb_audio_control);
>
Powered by blists - more mailing lists