On Sat, Jan 13, 2024 at 02:03:59AM +0200, Laurent Pinchart wrote:
> 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.
Actually, I think this should be fixed by catching the pipeline
misconfiguration at validation time, not when enabling streams. It would
make this patch redundant, but I think we can still merge it for
additional safety, especially as it may take more time to implement the
validation time check.
> 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@xxxxxxxxxxxxxxxx>
>
> > > > > Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
> > > > > Signed-off-by: Marek Vasut <marex@xxxxxxx>
> > > > > ---
> > > > > Cc: Fabio Estevam <festevam@xxxxxxxxx>
> > > > > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > > > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > > > > Cc: NXP Linux Team <linux-imx@xxxxxxx>
> > > > > Cc: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> > > > > Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > > > > Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
> > > > > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > > > > Cc: linux-media@xxxxxxxxxxxxxxx
> > > > > ---
> > > > > 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
