| Message ID | 20231006074654.11468-1-eagle.alexander923@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: imx-mipi-csis: Fix null pointer dereference when link is not set | expand |
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,
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,
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...
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.
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?
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 --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,
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(+)