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

[PATCH v3] 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.

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.

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

Fixes: ad85094b293e ("media: atomisp: gc2235: Remove driver")
Fixes: e838b8c69e45 ("media: atomisp: Drop intel_v4l2_subdev_type")
Signed-off-by: Yuho Choi <yqc5929@psu.edu>
---

Changes since v2:
- Replaced gc2235_remove() calls in remaining two error paths with
  goto labels to unwind only initialized resources
- Added Fixes tag

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

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
index d3414312e1de2..eedaedc84284b 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
@@ -818,18 +818,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 out_free;
 
 	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_free_ctrl;
 	}
 
 	/* Use same lock for controls as for everything else. */
@@ -837,13 +835,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;
+	}
 
-	return atomisp_register_i2c_module(&dev->sd, gcpdev);
+	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 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 v3] media: atomisp: gc2235: fix UAF and memory leak

[Andy Shevchenko]

On Wed, Apr 1, 2026 at 7:30 PM Yuho Choi <dbgh9129@gmail.com> 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.
>
> Handle each failure path with explicit unwind labels that free only
> what has been initialized. Return success only after the full probe
> sequence completes.

> Fixes: ad85094b293e ("media: atomisp: gc2235: Remove driver")

How does this fix the commit that states it removed the driver?
Actually the description is wrong. How on earth did you get above? My
simple Git command shows this:
ad85094b293e ("Revert "media: staging: atomisp: Remove driver"")

And even though it's wrong to have as Fixes tag. The driver was way
before that commit in the tree.

> Fixes: e838b8c69e45 ("media: atomisp: Drop intel_v4l2_subdev_type")

> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
> @@ -818,18 +818,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 out_free;
>
>         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_free_ctrl;
>         }
>
>         /* Use same lock for controls as for everything else. */
> @@ -837,13 +835,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;
> +       }
>
> -       return atomisp_register_i2c_module(&dev->sd, gcpdev);
> +       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 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)
>


-- 
With Best Regards,
Andy Shevchenko

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

[Dan Carpenter]

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

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

[최유호]

Dear Andy and Dan,

I appreciate your time and the detailed feedback.

To Andy:
My apologies for the incorrect "Fixes" tag description. I clearly
missed the "Revert" part in the
commit title. I will also re-examine the history and update it.

To Dan:
Thanks for the suggestion on the error handling path. I will refactor
the code to use direct
returns. I'll also ensure to use the "checkpatch.pl" this time.

I will submit a new patch addressing these points soon.

Best regards,
Yuho Choi


On Thu, 2 Apr 2026 at 04:41, Dan Carpenter <error27@gmail.com> wrote:
>
> 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;
>  }
>