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
signature.asc
Description: PGP signature
