| Message ID | 20231201150614.63300-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: nxp: isi: Check whether pad is non-NULL before access | expand |
On Fri, Dec 1, 2023 at 12:06 PM Marek Vasut <marex@denx.de> wrote: > 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, Maybe dev_err() here instead? Anyway: Reviewed-by: Fabio Estevam <festevam@gmail.com>
On 12/2/23 03:27, Fabio Estevam wrote: > On Fri, Dec 1, 2023 at 12:06 PM Marek Vasut <marex@denx.de> wrote: > >> 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, > > Maybe dev_err() here instead? That dev_dbg() is aligned with the prints in the rest of the function and it's not like kernel should do dev_err() into kernel log every time userspace does something wrong. > Anyway: > > Reviewed-by: Fabio Estevam <festevam@gmail.com> Thanks !
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 ? > 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",
On Sat, Dec 02, 2023 at 07:44:47AM +0100, Marek Vasut wrote: > On 12/2/23 03:27, Fabio Estevam wrote: > > On Fri, Dec 1, 2023 at 12:06 PM Marek Vasut <marex@denx.de> wrote: > > > >> 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, > > > > Maybe dev_err() here instead? > > That dev_dbg() is aligned with the prints in the rest of the function > and it's not like kernel should do dev_err() into kernel log every time > userspace does something wrong. We usually use dev_dbg() for errors that can be easily triggered from userspace, to avoid giving unpriviledged processes an easy way to flood the kernel log. > > Anyway: > > > > Reviewed-by: Fabio Estevam <festevam@gmail.com> > > Thanks !
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'. -- Kieran > > > 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
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). > > > 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
Quoting Laurent Pinchart (2023-12-03 17:00:57) > On Sat, Dec 02, 2023 at 07:44:47AM +0100, Marek Vasut wrote: > > On 12/2/23 03:27, Fabio Estevam wrote: > > > On Fri, Dec 1, 2023 at 12:06 PM Marek Vasut <marex@denx.de> wrote: > > > > > >> 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, > > > > > > Maybe dev_err() here instead? > > > > That dev_dbg() is aligned with the prints in the rest of the function > > and it's not like kernel should do dev_err() into kernel log every time > > userspace does something wrong. > > We usually use dev_dbg() for errors that can be easily triggered from > userspace, to avoid giving unpriviledged processes an easy way to flood > the kernel log. When this call returns it will also report a dev_err "Failed to enable pipe": [ 56.806321] mxc-isi 32e00000.isi: no pad connecxted to crossbar input 0 [ 56.813655] mxc-isi 32e00000.isi: Failed to enable pipe 0 Personally - I'd make this print dev_err so the /reason/ we failed to enable the pipe is also stated. But I can confirm this is a bug and fairly easily trigged by attempting to capture from the ISI through the libcamera simple pipeline handler, which (I believe) does not do any route handling at present. Which ever log level: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> And this needs to go to stable, as this caused a null-pointer-deref for me on a v6.6 tree. -- Kieran > > > > Anyway: > > > > > > Reviewed-by: Fabio Estevam <festevam@gmail.com> > > > > Thanks ! > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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",
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@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",
On 1/13/24 01:03, 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. > > 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> Sure, go for it.
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",
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. 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(-)