Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]

Hi Benoit,

Thanks for your comments,

On Tue, Sep 12, 2017 at 01:23:39PM -0500, Benoit Parrot wrote:
> > +static int csi2rx_probe(struct platform_device *pdev)
> > +{
> > +	struct v4l2_async_subdev **subdevs;
> > +	struct csi2rx_priv *csi2rx;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/*
> > +	 * Since the v4l2_subdev structure is embedded in our
> > +	 * csi2rx_priv structure, and that the structure is exposed to
> > +	 * the user-space, we cannot just use the devm_variant
> > +	 * here. Indeed, that would lead to a use-after-free in a
> > +	 * open() - unbind - close() pattern.
> > +	 */
> > +	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
> > +	if (!csi2rx)
> > +		return -ENOMEM;
> > +	platform_set_drvdata(pdev, csi2rx);
> > +	csi2rx->dev = &pdev->dev;

[snip]

> > +
> > +	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> > +	if (!subdevs) {
> > +		ret = -ENOMEM;
> > +		goto err_free_priv;
> > +	}
> > +	subdevs[0] = &csi2rx->asd;
> > +
> 
> Shouldn't the comment related to lifetime of memory allocation be
> also applied here?  A reference to the "subdevs" pointer is taken
> internally so it might suffer the same fate.  Not sure how many
> "struct v4l2_async_subdev **subdevs" we would end up needing but
> since here we are only dealing with one, why not just make it a
> member of the struct csi2rx_priv object.

As far as I know, only the notifier will use that array. The notifier
will be removed before that array is de-allocated, and the user-space
never has access to it, so I'm not sure the same issue arises here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]