Messages in this thread | | | Subject | Re: [PATCH] bus: fsl-mc: fix improper free of mc_dev | From | Tom Rix <> | Date | Wed, 28 Apr 2021 10:26:16 -0700 |
| |
On 4/27/21 12:19 PM, Nick Desaulniers wrote: > On Tue, Apr 27, 2021 at 11:36 AM <trix@redhat.com> wrote: >> From: Tom Rix <trix@redhat.com> >> >> Clang static analysis reports this error >> >> fsl-mc-bus.c:891:2: warning: Attempt to free released memory >> kfree(mc_dev); >> ^~~~~~~~~~~~~ >> >> In this block of code >> >> if (strcmp(obj_desc->type, "dprc") == 0) { >> .. >> mc_bus = kzalloc(..) >> mc_dev = &mc_bus->mc_dev; > Thanks for the patch. > > Aren't the allocations for mc_bus and mc_dev mutually exclusive based > on that conditional? If so... > >> mc_dev is not alloc-ed, so it should not be freed. >> Old handler triggers a false positive from checkpatch, so add a >> comment and change logic a bit. >> >> Fixes: a042fbed0290 ("staging: fsl-mc: simplify couple of deallocations") >> Signed-off-by: Tom Rix <trix@redhat.com> >> --- >> drivers/bus/fsl-mc/fsl-mc-bus.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c >> index 380ad1fdb745..fb3e1d8a7f63 100644 >> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c >> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c >> @@ -887,8 +887,10 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc, >> >> error_cleanup_dev: >> kfree(mc_dev->regions); >> + /* mc_dev is only allocated when it is not part of mc_bus */ >> + if (!mc_bus) >> + kfree(mc_dev); >> kfree(mc_bus); >> - kfree(mc_dev); > The error handling here seems quite wrong (regardless of your patch). > mc_dev->regions is allocated by fsl_mc_device_get_mmio_regions() IIUC. > Wouldn't the first `goto error_cleanup_dev;` taken end up passing an > uninitialized pointer to kfree()?
On the first goto, mc_dev->regions, because of the kzalloc, the value would be
mc_bus->mc_dev.regions , should be 0 or
mc_dev->regions, which also should be 0
and kfree handles 0.
> > what if `strcmp(obj_desc->type, "dprc") == 0` is false? We allocate > `mc_dev`, but then call kfree on `mc_bus`?
mc_bus is initialized to NULL, which makes the call to kfree safe.
The original handler was
if (mc_bus)
kfree(mc_bus)
else
kfree(mc_dev)
I tried this first, which works, but checkpatch throw a warning for kfree(mc_bus).
This change makes the 'else' with the !mc_bus
> I think it would be safer to locally save the result of > `strcmp(obj_desc->type, "dprc") == 0`, then check that throughout this the local mc_bus is only set in this block, so I don't think another local is needed. > function, including the error handling at the end, or use multiple > labels to unwind the allocations correctly.
The goto's could be finer grained because some of the mc_dev->regions are known to be unallocated.
Changing these would not be a fix and it could be argued the simpler, less efficent error handling works as designed.
Tom
> >> return error; >> } >> -- > > -- > Thanks, > ~Nick Desaulniers >
| |