Re: [PATCH v3] media: atomisp: gc2235: fix UAF and memory leak

[Dan Carpenter <error27@gmail.com>]

Please run your patches through checkpatch.pl.

On Wed, Apr 01, 2026 at 12:30:50PM -0400, Yuho Choi wrote:
> gc2235_probe() handles its error paths incorrectly.
> 
> If media_entity_pads_init() fails, gc2235_remove() is called, which
> tears down the subdev and frees dev, but then still falls through to
> atomisp_register_i2c_module(). This results in use-after-free.
> 
> If atomisp_register_i2c_module() fails, the media entity and control
> handler are left initialized and dev is leaked.
> 
> gc2235_remove() is the full teardown path for a successfully probed
> device; it unconditionally assumes a fully-initialized device.
> gc2235_probe() must unwind only the resources that were actually
> initialized at the point of failure.

The "must unwind only the resources that were actually initialized at
the point of failure." phrasing is too strong. I was hoping you would
review it and find an actual bug.  I reviewed it myself and didn't find
a bug beyond the leaks and use after frees mentioned in this commit
message.  As I wrote in my blog, leaks are one of the common bugs from
this style of error handling because it is too complicated.

> 
> Handle each failure path with explicit unwind labels that free only
> what has been initialized. Return success only after the full probe
> sequence completes.

If I were determined to use a magical cleanup function to do the cleanups
then I would reverse the gotos and direct returns.

regards,
dan carpenter

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
index d3414312e1de..61fb82b26cc9 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
@@ -808,8 +808,11 @@ static int gc2235_probe(struct i2c_client *client)
 					   atomisp_bayer_order_grbg);
 
 	ret = gc2235_s_config(&dev->sd, client->irq, gcpdev);
-	if (ret)
-		goto out_free;
+	if (ret) {
+		v4l2_device_unregister_subdev(&dev->sd);
+		kfree(dev);
+		return ret;
+	}
 
 	dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	dev->pad.flags = MEDIA_PAD_FL_SOURCE;
@@ -818,18 +821,16 @@ static int gc2235_probe(struct i2c_client *client)
 	ret =
 	    v4l2_ctrl_handler_init(&dev->ctrl_handler,
 				   ARRAY_SIZE(gc2235_controls));
-	if (ret) {
-		gc2235_remove(client);
-		return ret;
-	}
+	if (ret)
+		goto err_remove;
 
 	for (i = 0; i < ARRAY_SIZE(gc2235_controls); i++)
 		v4l2_ctrl_new_custom(&dev->ctrl_handler, &gc2235_controls[i],
 				     NULL);
 
 	if (dev->ctrl_handler.error) {
-		gc2235_remove(client);
-		return dev->ctrl_handler.error;
+		ret = dev->ctrl_handler.error;
+		goto err_remove;
 	}
 
 	/* Use same lock for controls as for everything else. */
@@ -838,14 +839,16 @@ static int gc2235_probe(struct i2c_client *client)
 
 	ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad);
 	if (ret)
-		gc2235_remove(client);
+		goto err_remove;
 
-	return atomisp_register_i2c_module(&dev->sd, gcpdev);
+	ret = atomisp_register_i2c_module(&dev->sd, gcpdev);
+	if (ret)
+		goto err_remove;
 
-out_free:
-	v4l2_device_unregister_subdev(&dev->sd);
-	kfree(dev);
+	return 0;
 
+err_remove:
+	gc2235_remove(client);
 	return ret;
 }