Re: [PATCH] bus: fsl-mc: Fix the double free in fsl_mc_device_add()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024/11/14 16:57, Dan Carpenter wrote:
On Thu, Nov 14, 2024 at 11:41:25AM +0300, Dan Carpenter wrote:
On Thu, Nov 14, 2024 at 04:27:52PM +0800, Su Hui wrote:
Clang static checker(scan-build) warning:
drivers/bus/fsl-mc/fsl-mc-bus.c: line 909, column 2
Attempt to free released memory.

When 'obj_desc->type' == "dprc" and begin to free 'mc_bus' and 'mc_dev',
there is a double free problem because of 'mc_dev = &mc_bus->mc_dev'.
Add a judgment to fix this problem.

Fixes: a042fbed0290 ("staging: fsl-mc: simplify couple of deallocations")
Signed-off-by: Su Hui <suhui@xxxxxxxxxxxx>
---
  drivers/bus/fsl-mc/fsl-mc-bus.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 930d8a3ba722..8d2d5d3cc782 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -905,8 +905,10 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc *obj_desc,
error_cleanup_dev:
  	kfree(mc_dev->regions);
-	kfree(mc_bus);
-	kfree(mc_dev);
+	if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
This works, but it would probably be nicer to write this as:

	if (is_fsl_mc_bus_dprc(mc_dev))
		kfree(mc_bus);
	else
		kfree(mc_dev);

That way it would match the release function.
Yes, it's better!
    820          mc_dev->dev.release = fsl_mc_device_release;

	[ snip ]

    891           * The device-specific probe callback will get invoked by device_add()
    892           */
    893          error = device_add(&mc_dev->dev);
    894          if (error < 0) {
    895                  dev_err(parent_dev,
    896                          "device_add() failed for device %s: %d\n",
    897                          dev_name(&mc_dev->dev), error);
    898                  goto error_cleanup_dev;

I don't think this goto is correct.  I think fsl_mc_device_release() will be
called automaticall on this path so the goto is a double free.
Agreed too, maybe using put_device(&mc_dev->dev) to replace ?

    899          }
    900
    901          dev_dbg(parent_dev, "added %s\n", dev_name(&mc_dev->dev));
    902
    903          *new_mc_dev = mc_dev;
    904          return 0;
    905
    906  error_cleanup_dev:
    907          kfree(mc_dev->regions);
    908          if (is_fsl_mc_bus_dprc(mc_dev))

Yep, this looks better :)

I will send a v2 patch to use is_fsl_mc_bus_dprc().
It might take some time because I'm not sure about using
'put_device(&mc_dev->dev)' to replace 'goto error_cleanup_dev'.

regards,
Su Hui





[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux