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