diff mbox series

media: imx: imx-mipi-csis: Fix null pointer dereference when link is not set

Message ID 20231006074654.11468-1-eagle.alexander923@gmail.com
State New
Headers show
Series media: imx: imx-mipi-csis: Fix null pointer dereference when link is not set | expand

Commit Message

Alexander Shiyan Oct. 6, 2023, 7:46 a.m. UTC
Let's add a check for src_sd before using it.
The link may not be set, in which case the call to this function will fail.

Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
---
 drivers/media/platform/nxp/imx-mipi-csis.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Sakari Ailus Oct. 11, 2023, 7:44 p.m. UTC | #1
Hi Alexander,

On Fri, Oct 06, 2023 at 10:46:54AM +0300, Alexander Shiyan wrote:
> Let's add a check for src_sd before using it.
> The link may not be set, in which case the call to this function will fail.

That would seem like an understatement.

Any idea when this was introduced (and which patch did), Fixes: and Cc:
stable should be added if this is already in a release.

> 
> Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 5f93712bf485..df5a159b2d39 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -595,6 +595,9 @@ static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
>  	s64 link_freq;
>  	u32 lane_rate;
>  
> +	if (!csis->src_sd)
> +		return -EINVAL;
> +
>  	/* Calculate the line rate from the pixel rate. */
>  	link_freq = v4l2_get_link_freq(csis->src_sd->ctrl_handler,
>  				       csis_fmt->width,
Laurent Pinchart Oct. 11, 2023, 7:48 p.m. UTC | #2
On Wed, Oct 11, 2023 at 07:44:59PM +0000, Sakari Ailus wrote:
> On Fri, Oct 06, 2023 at 10:46:54AM +0300, Alexander Shiyan wrote:
> > Let's add a check for src_sd before using it.
> > The link may not be set, in which case the call to this function will fail.
> 
> That would seem like an understatement.
> 
> Any idea when this was introduced (and which patch did), Fixes: and Cc:
> stable should be added if this is already in a release.

It's actually an issue in the pipeline validation code in the V4L2 core.
The link is marked as MUST_CONNECT, but that isn't handled properly :-(
It's been on my todo list for a while but I haven't had time to get to
it. Feel free to give it a go.

> > Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.com>
> > ---
> >  drivers/media/platform/nxp/imx-mipi-csis.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> > index 5f93712bf485..df5a159b2d39 100644
> > --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> > +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> > @@ -595,6 +595,9 @@ static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
> >  	s64 link_freq;
> >  	u32 lane_rate;
> >  
> > +	if (!csis->src_sd)
> > +		return -EINVAL;
> > +
> >  	/* Calculate the line rate from the pixel rate. */
> >  	link_freq = v4l2_get_link_freq(csis->src_sd->ctrl_handler,
> >  				       csis_fmt->width,
Sakari Ailus Oct. 11, 2023, 8:01 p.m. UTC | #3
On Wed, Oct 11, 2023 at 10:48:33PM +0300, Laurent Pinchart wrote:
> On Wed, Oct 11, 2023 at 07:44:59PM +0000, Sakari Ailus wrote:
> > On Fri, Oct 06, 2023 at 10:46:54AM +0300, Alexander Shiyan wrote:
> > > Let's add a check for src_sd before using it.
> > > The link may not be set, in which case the call to this function will fail.
> > 
> > That would seem like an understatement.
> > 
> > Any idea when this was introduced (and which patch did), Fixes: and Cc:
> > stable should be added if this is already in a release.
> 
> It's actually an issue in the pipeline validation code in the V4L2 core.
> The link is marked as MUST_CONNECT, but that isn't handled properly :-(
> It's been on my todo list for a while but I haven't had time to get to
> it. Feel free to give it a go.

What's wrong there? It used to work at least...
Laurent Pinchart Oct. 11, 2023, 8:06 p.m. UTC | #4
On Wed, Oct 11, 2023 at 08:01:50PM +0000, Sakari Ailus wrote:
> On Wed, Oct 11, 2023 at 10:48:33PM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 11, 2023 at 07:44:59PM +0000, Sakari Ailus wrote:
> > > On Fri, Oct 06, 2023 at 10:46:54AM +0300, Alexander Shiyan wrote:
> > > > Let's add a check for src_sd before using it.
> > > > The link may not be set, in which case the call to this function will fail.
> > > 
> > > That would seem like an understatement.
> > > 
> > > Any idea when this was introduced (and which patch did), Fixes: and Cc:
> > > stable should be added if this is already in a release.
> > 
> > It's actually an issue in the pipeline validation code in the V4L2 core.
> > The link is marked as MUST_CONNECT, but that isn't handled properly :-(
> > It's been on my todo list for a while but I haven't had time to get to
> > it. Feel free to give it a go.
> 
> What's wrong there? It used to work at least...

It's called a regression :-) If I recall correctly, if the pad is not
connected, it's not added to the list of pads to check, and the
MUST_CONNECT flag is not checked.
Sakari Ailus Oct. 11, 2023, 8:19 p.m. UTC | #5
On Wed, Oct 11, 2023 at 11:06:15PM +0300, Laurent Pinchart wrote:
> On Wed, Oct 11, 2023 at 08:01:50PM +0000, Sakari Ailus wrote:
> > On Wed, Oct 11, 2023 at 10:48:33PM +0300, Laurent Pinchart wrote:
> > > On Wed, Oct 11, 2023 at 07:44:59PM +0000, Sakari Ailus wrote:
> > > > On Fri, Oct 06, 2023 at 10:46:54AM +0300, Alexander Shiyan wrote:
> > > > > Let's add a check for src_sd before using it.
> > > > > The link may not be set, in which case the call to this function will fail.
> > > > 
> > > > That would seem like an understatement.
> > > > 
> > > > Any idea when this was introduced (and which patch did), Fixes: and Cc:
> > > > stable should be added if this is already in a release.
> > > 
> > > It's actually an issue in the pipeline validation code in the V4L2 core.
> > > The link is marked as MUST_CONNECT, but that isn't handled properly :-(
> > > It's been on my todo list for a while but I haven't had time to get to
> > > it. Feel free to give it a go.
> > 
> > What's wrong there? It used to work at least...
> 
> It's called a regression :-) If I recall correctly, if the pad is not
> connected, it's not added to the list of pads to check, and the
> MUST_CONNECT flag is not checked.

This has potential for other similar issues in a number of drivers. I
suppose this broke with the graph traversal changes ~ a year ago?
Laurent Pinchart Oct. 11, 2023, 8:21 p.m. UTC | #6
On Wed, Oct 11, 2023 at 08:19:03PM +0000, Sakari Ailus wrote:
> On Wed, Oct 11, 2023 at 11:06:15PM +0300, Laurent Pinchart wrote:
> > On Wed, Oct 11, 2023 at 08:01:50PM +0000, Sakari Ailus wrote:
> > > On Wed, Oct 11, 2023 at 10:48:33PM +0300, Laurent Pinchart wrote:
> > > > On Wed, Oct 11, 2023 at 07:44:59PM +0000, Sakari Ailus wrote:
> > > > > On Fri, Oct 06, 2023 at 10:46:54AM +0300, Alexander Shiyan wrote:
> > > > > > Let's add a check for src_sd before using it.
> > > > > > The link may not be set, in which case the call to this function will fail.
> > > > > 
> > > > > That would seem like an understatement.
> > > > > 
> > > > > Any idea when this was introduced (and which patch did), Fixes: and Cc:
> > > > > stable should be added if this is already in a release.
> > > > 
> > > > It's actually an issue in the pipeline validation code in the V4L2 core.
> > > > The link is marked as MUST_CONNECT, but that isn't handled properly :-(
> > > > It's been on my todo list for a while but I haven't had time to get to
> > > > it. Feel free to give it a go.
> > > 
> > > What's wrong there? It used to work at least...
> > 
> > It's called a regression :-) If I recall correctly, if the pad is not
> > connected, it's not added to the list of pads to check, and the
> > MUST_CONNECT flag is not checked.
> 
> This has potential for other similar issues in a number of drivers.

Indeed, which is why it should be fixed there.

> I suppose this broke with the graph traversal changes ~ a year ago?

I assume so, yes.
diff mbox series

Patch

diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
index 5f93712bf485..df5a159b2d39 100644
--- a/drivers/media/platform/nxp/imx-mipi-csis.c
+++ b/drivers/media/platform/nxp/imx-mipi-csis.c
@@ -595,6 +595,9 @@  static int mipi_csis_calculate_params(struct mipi_csis_device *csis,
 	s64 link_freq;
 	u32 lane_rate;
 
+	if (!csis->src_sd)
+		return -EINVAL;
+
 	/* Calculate the line rate from the pixel rate. */
 	link_freq = v4l2_get_link_freq(csis->src_sd->ctrl_handler,
 				       csis_fmt->width,