Re: [PATCH] bus: fsl-mc: fix improper free of mc_dev

From: Tom Rix
Date: Wed Apr 28 2021 - 13:26:29 EST



On 4/27/21 12:19 PM, Nick Desaulniers wrote:
On Tue, Apr 27, 2021 at 11:36 AM <trix@xxxxxxxxxx> wrote:
From: Tom Rix <trix@xxxxxxxxxx>

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@xxxxxxxxxx>
---
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