Re: [PATCH] media: nxp: isi: Check whether pad is non-NULL before access

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, Jan 11, 2024 at 07:52:33PM +0200, Laurent Pinchart wrote:
> On Thu, Jan 11, 2024 at 05:49:41PM +0000, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2023-12-03 16:59:59)
> > > Hi Marek,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Fri, Dec 01, 2023 at 04:06:04PM +0100, Marek Vasut wrote:
> > > > The pad can be NULL if media controller routing is not set up correctly.
> > > > Check whether the pad is NULL before using it, otherwise it is possible
> > > > to achieve NULL pointer dereference.
> > > 
> > > Could you share more information about how to misconfigure the routing ?
> > 
> > You simply do 'nothing'.
> 
> The default configuration should be working one. I think that should
> then be fixed too (in a separate patch).

I managed to reproduce the issue (I had to heavily hack libcamera, by
default it would reject incorrect configurations before triggering the
kernel bug). The default configuration of the crossbar switch is fine,
this patch is the right fix.

I'd like to expand the commit message with a clearer description of the
erronous configuration. Marek, are you fine with the following commit
message ?

--------
When translating source to sink streams in the crossbar subdev, the
driver tries to locate the remote subdev connected to the sink pad. The
remote pad may be NULL, if userspace tries to enable a stream that ends
at an unconnected crossbar sink. When that occurs, the driver
dereferences the NULL pad, leading to a crash.

Prevent the crash by checking if the pad is NULL before using it, and
return an error if it is.
--------

If so I'll update it locally, no need for a new version.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > > Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
> > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > ---
> > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > Cc: linux-media@vger.kernel.org
> > > > ---
> > > >  drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > > > index 792f031e032ae..44354931cf8a1 100644
> > > > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > > > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > > > @@ -160,8 +160,14 @@ mxc_isi_crossbar_xlate_streams(struct mxc_isi_crossbar *xbar,
> > > >       }
> > > >  
> > > >       pad = media_pad_remote_pad_first(&xbar->pads[sink_pad]);
> > > > -     sd = media_entity_to_v4l2_subdev(pad->entity);
> > > > +     if (!pad) {
> > > > +             dev_dbg(xbar->isi->dev,
> > > > +                     "no pad connected to crossbar input %u\n",
> > > > +                     sink_pad);
> > > > +             return ERR_PTR(-EPIPE);
> > > > +     }
> > > >  
> > > > +     sd = media_entity_to_v4l2_subdev(pad->entity);
> > > >       if (!sd) {
> > > >               dev_dbg(xbar->isi->dev,
> > > >                       "no entity connected to crossbar input %u\n",

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel