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

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

[Yuho Choi]

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.

Handle each failure path locally and unwind only the initialized
resources. Return success only after the full probe sequence completes.

Signed-off-by: Yuho Choi <yqc5929@psu.edu>
---

Changes since v1:
- Edited the commit message to be imperative mood
- Corrected the previous mangled patch

 .../media/atomisp/i2c/atomisp-gc2235.c        | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
index d3414312e1de2..f4eb15d307fae 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
@@ -837,13 +837,24 @@ static int gc2235_probe(struct i2c_client *client)
 	dev->sd.ctrl_handler = &dev->ctrl_handler;
 
 	ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad);
-	if (ret)
-		gc2235_remove(client);
+	if (ret) {
+		dev_err(&client->dev, "media_entity_pads_init failed\n");
+		goto err_free_ctrl;
+	}
+
+	ret = atomisp_register_i2c_module(&dev->sd, gcpdev);
+	if (ret) {
+		dev_err(&client->dev, "atomisp_register_i2c_module failed\n");
+		goto err_entity_cleanup;
+	}
 
-	return atomisp_register_i2c_module(&dev->sd, gcpdev);
+	return 0;
 
+err_entity_cleanup:
+	media_entity_cleanup(&dev->sd.entity);
+err_free_ctrl:
+	v4l2_ctrl_handler_free(&dev->ctrl_handler);
 out_free:
-	v4l2_device_unregister_subdev(&dev->sd);
 	kfree(dev);
 
 	return ret;
-- 
2.50.1 (Apple Git-155)

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

[Dan Carpenter]

On Tue, Mar 31, 2026 at 03:47:27PM -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.
> 
> Handle each failure path locally and unwind only the initialized
> resources. Return success only after the full probe sequence completes.
> 
> Signed-off-by: Yuho Choi <yqc5929@psu.edu>

Needs a Fixes tag.

The design of this code is that gc2235_remove() is supposed to
clean up from every type of possible error.  This style of
error handling is always buggy:
https://staticthinking.wordpress.com/2025/03/31/reviewing-magical-cleanup-functions/

However, the commit message does not explain why it's buggy
in this case and there are still two error paths that use gc2235_remove()
to clean up.  Please, could you fix those two error paths and explain why
we cannot just do:

	ret = atomisp_register_i2c_module();
	if (ret)
		goto out_free;

	return 0;

out_free:
	gc2235_remove(client);
	return ret;

regards,
dan carpenter