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