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

Dan Carpenter
2026-04-02T08:41:37Z

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;
}