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

[Dan Carpenter <error27@gmail.com>]

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