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

최유호
2026-04-02T15:30:14Z

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