* [PATCH] bus: fsl-mc: fix improper free of mc_dev
@ 2021-04-27 18:36 trix
2021-04-27 19:19 ` Nick Desaulniers
0 siblings, 1 reply; 3+ messages in thread
From: trix @ 2021-04-27 18:36 UTC (permalink / raw)
To: stuyoder, laurentiu.tudor, nathan, ndesaulniers, gregkh
Cc: linux-kernel, clang-built-linux, Tom Rix
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;
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);
return error;
}
--
2.26.3
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] bus: fsl-mc: fix improper free of mc_dev 2021-04-27 18:36 [PATCH] bus: fsl-mc: fix improper free of mc_dev trix @ 2021-04-27 19:19 ` Nick Desaulniers 2021-04-28 17:26 ` Tom Rix 0 siblings, 1 reply; 3+ messages in thread From: Nick Desaulniers @ 2021-04-27 19:19 UTC (permalink / raw) To: Tom Rix Cc: stuyoder, laurentiu.tudor, Nathan Chancellor, Greg KH, LKML, clang-built-linux 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()? what if `strcmp(obj_desc->type, "dprc") == 0` is false? We allocate `mc_dev`, but then call kfree on `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 function, including the error handling at the end, or use multiple labels to unwind the allocations correctly. > > return error; > } > -- -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] bus: fsl-mc: fix improper free of mc_dev 2021-04-27 19:19 ` Nick Desaulniers @ 2021-04-28 17:26 ` Tom Rix 0 siblings, 0 replies; 3+ messages in thread From: Tom Rix @ 2021-04-28 17:26 UTC (permalink / raw) To: Nick Desaulniers Cc: stuyoder, laurentiu.tudor, Nathan Chancellor, Greg KH, LKML, clang-built-linux 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-28 17:26 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-27 18:36 [PATCH] bus: fsl-mc: fix improper free of mc_dev trix 2021-04-27 19:19 ` Nick Desaulniers 2021-04-28 17:26 ` Tom Rix
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).