[PATCH] ipu3-imgu: Fix NULL pointer dereference in imgu_subdev_set_selection()

[PATCH] ipu3-imgu: Fix NULL pointer dereference in imgu_subdev_set_selection()

[Maximilian Luz]

Calling v4l2_subdev_get_try_crop() and v4l2_subdev_get_try_compose()
with a subdev state of NULL leads to a NULL pointer dereference. This
can currently happen in imgu_subdev_set_selection() when the state
passed in is NULL, as this method first gets pointers to both the "try"
and "active" states and only then decides which to use.

The same issue has been addressed for imgu_subdev_get_selection() with
commit 30d03a0de650 ("ipu3-imgu: Fix NULL pointer dereference in active
selection access"). However the issue still persists in
imgu_subdev_set_selection().

Therefore, apply a similar fix as done in the aforementioned commit to
imgu_subdev_set_selection(). To keep things a bit cleaner, introduce
helper functions for "crop" and "compose" access and use them in both
imgu_subdev_set_selection() and imgu_subdev_get_selection().

Fixes: 0d346d2a6f54 ("media: v4l2-subdev: add subdev-wide state struct")
Cc: stable@vger.kernel.org # for v5.14 and later
Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
---
 drivers/staging/media/ipu3/ipu3-v4l2.c | 57 +++++++++++++++-----------
 1 file changed, 34 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index ce13e746c15f..e530767e80a5 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -188,6 +188,28 @@ static int imgu_subdev_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static struct v4l2_rect *
+imgu_subdev_get_crop(struct imgu_v4l2_subdev *sd,
+		     struct v4l2_subdev_state *sd_state, unsigned int pad,
+		     enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_crop(&sd->subdev, sd_state, pad);
+	else
+		return &sd->rect.eff;
+}
+
+static struct v4l2_rect *
+imgu_subdev_get_compose(struct imgu_v4l2_subdev *sd,
+			struct v4l2_subdev_state *sd_state, unsigned int pad,
+			enum v4l2_subdev_format_whence which)
+{
+	if (which == V4L2_SUBDEV_FORMAT_TRY)
+		return v4l2_subdev_get_try_compose(&sd->subdev, sd_state, pad);
+	else
+		return &sd->rect.bds;
+}
+
 static int imgu_subdev_get_selection(struct v4l2_subdev *sd,
 				     struct v4l2_subdev_state *sd_state,
 				     struct v4l2_subdev_selection *sel)
@@ -200,18 +222,12 @@ static int imgu_subdev_get_selection(struct v4l2_subdev *sd,
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
-		if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
-			sel->r = *v4l2_subdev_get_try_crop(sd, sd_state,
-							   sel->pad);
-		else
-			sel->r = imgu_sd->rect.eff;
+		sel->r = *imgu_subdev_get_crop(imgu_sd, sd_state, sel->pad,
+					       sel->which);
 		return 0;
 	case V4L2_SEL_TGT_COMPOSE:
-		if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
-			sel->r = *v4l2_subdev_get_try_compose(sd, sd_state,
-							      sel->pad);
-		else
-			sel->r = imgu_sd->rect.bds;
+		sel->r = *imgu_subdev_get_compose(imgu_sd, sd_state, sel->pad,
+						  sel->which);
 		return 0;
 	default:
 		return -EINVAL;
@@ -223,10 +239,9 @@ static int imgu_subdev_set_selection(struct v4l2_subdev *sd,
 				     struct v4l2_subdev_selection *sel)
 {
 	struct imgu_device *imgu = v4l2_get_subdevdata(sd);
-	struct imgu_v4l2_subdev *imgu_sd = container_of(sd,
-							struct imgu_v4l2_subdev,
-							subdev);
-	struct v4l2_rect *rect, *try_sel;
+	struct imgu_v4l2_subdev *imgu_sd =
+		container_of(sd, struct imgu_v4l2_subdev, subdev);
+	struct v4l2_rect *rect;
 
 	dev_dbg(&imgu->pci_dev->dev,
 		 "set subdev %u sel which %u target 0x%4x rect [%ux%u]",
@@ -238,22 +253,18 @@ static int imgu_subdev_set_selection(struct v4l2_subdev *sd,
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
-		try_sel = v4l2_subdev_get_try_crop(sd, sd_state, sel->pad);
-		rect = &imgu_sd->rect.eff;
+		rect = imgu_subdev_get_crop(imgu_sd, sd_state, sel->pad,
+					    sel->which);
 		break;
 	case V4L2_SEL_TGT_COMPOSE:
-		try_sel = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
-		rect = &imgu_sd->rect.bds;
+		rect = imgu_subdev_get_compose(imgu_sd, sd_state, sel->pad,
+					       sel->which);
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
-		*try_sel = sel->r;
-	else
-		*rect = sel->r;
-
+	*rect = sel->r;
 	return 0;
 }
 
-- 
2.37.3

Re: [PATCH] ipu3-imgu: Fix NULL pointer dereference in imgu_subdev_set_selection()

[Tomi Valkeinen]

On 08/09/2022 01:44, Maximilian Luz wrote:
> Calling v4l2_subdev_get_try_crop() and v4l2_subdev_get_try_compose()
> with a subdev state of NULL leads to a NULL pointer dereference. This
> can currently happen in imgu_subdev_set_selection() when the state
> passed in is NULL, as this method first gets pointers to both the "try"
> and "active" states and only then decides which to use.
> 
> The same issue has been addressed for imgu_subdev_get_selection() with
> commit 30d03a0de650 ("ipu3-imgu: Fix NULL pointer dereference in active
> selection access"). However the issue still persists in
> imgu_subdev_set_selection().
> 
> Therefore, apply a similar fix as done in the aforementioned commit to
> imgu_subdev_set_selection(). To keep things a bit cleaner, introduce
> helper functions for "crop" and "compose" access and use them in both
> imgu_subdev_set_selection() and imgu_subdev_get_selection().
> 
> Fixes: 0d346d2a6f54 ("media: v4l2-subdev: add subdev-wide state struct")
> Cc: stable@vger.kernel.org # for v5.14 and later
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>   drivers/staging/media/ipu3/ipu3-v4l2.c | 57 +++++++++++++++-----------
>   1 file changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index ce13e746c15f..e530767e80a5 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -188,6 +188,28 @@ static int imgu_subdev_set_fmt(struct v4l2_subdev *sd,
>   	return 0;
>   }
>   
> +static struct v4l2_rect *
> +imgu_subdev_get_crop(struct imgu_v4l2_subdev *sd,
> +		     struct v4l2_subdev_state *sd_state, unsigned int pad,
> +		     enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return v4l2_subdev_get_try_crop(&sd->subdev, sd_state, pad);
> +	else
> +		return &sd->rect.eff;
> +}
> +
> +static struct v4l2_rect *
> +imgu_subdev_get_compose(struct imgu_v4l2_subdev *sd,
> +			struct v4l2_subdev_state *sd_state, unsigned int pad,
> +			enum v4l2_subdev_format_whence which)
> +{
> +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> +		return v4l2_subdev_get_try_compose(&sd->subdev, sd_state, pad);
> +	else
> +		return &sd->rect.bds;
> +}


If I understand right, these functions are only called with pad 0 
(IMGU_NODE_IN). I would drop the pad argument here and use IMGU_NODE_IN. 
Otherwise it gives a false idea that other pads could be used with these 
functions, and that would fail for the V4L2_SUBDEV_FORMAT_ACTIVE case.

However, that's not a big issue. With or without the change:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH] ipu3-imgu: Fix NULL pointer dereference in imgu_subdev_set_selection()

[Laurent Pinchart]

On Thu, Sep 08, 2022 at 03:08:27PM +0300, Tomi Valkeinen wrote:
> On 08/09/2022 01:44, Maximilian Luz wrote:
> > Calling v4l2_subdev_get_try_crop() and v4l2_subdev_get_try_compose()
> > with a subdev state of NULL leads to a NULL pointer dereference. This
> > can currently happen in imgu_subdev_set_selection() when the state
> > passed in is NULL, as this method first gets pointers to both the "try"
> > and "active" states and only then decides which to use.
> > 
> > The same issue has been addressed for imgu_subdev_get_selection() with
> > commit 30d03a0de650 ("ipu3-imgu: Fix NULL pointer dereference in active
> > selection access"). However the issue still persists in
> > imgu_subdev_set_selection().
> > 
> > Therefore, apply a similar fix as done in the aforementioned commit to
> > imgu_subdev_set_selection(). To keep things a bit cleaner, introduce
> > helper functions for "crop" and "compose" access and use them in both
> > imgu_subdev_set_selection() and imgu_subdev_get_selection().
> > 
> > Fixes: 0d346d2a6f54 ("media: v4l2-subdev: add subdev-wide state struct")
> > Cc: stable@vger.kernel.org # for v5.14 and later
> > Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> > ---
> >   drivers/staging/media/ipu3/ipu3-v4l2.c | 57 +++++++++++++++-----------
> >   1 file changed, 34 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > index ce13e746c15f..e530767e80a5 100644
> > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > @@ -188,6 +188,28 @@ static int imgu_subdev_set_fmt(struct v4l2_subdev *sd,
> >   	return 0;
> >   }
> >   
> > +static struct v4l2_rect *
> > +imgu_subdev_get_crop(struct imgu_v4l2_subdev *sd,
> > +		     struct v4l2_subdev_state *sd_state, unsigned int pad,
> > +		     enum v4l2_subdev_format_whence which)
> > +{
> > +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> > +		return v4l2_subdev_get_try_crop(&sd->subdev, sd_state, pad);
> > +	else
> > +		return &sd->rect.eff;
> > +}
> > +
> > +static struct v4l2_rect *
> > +imgu_subdev_get_compose(struct imgu_v4l2_subdev *sd,
> > +			struct v4l2_subdev_state *sd_state, unsigned int pad,
> > +			enum v4l2_subdev_format_whence which)
> > +{
> > +	if (which == V4L2_SUBDEV_FORMAT_TRY)
> > +		return v4l2_subdev_get_try_compose(&sd->subdev, sd_state, pad);
> > +	else
> > +		return &sd->rect.bds;
> > +}
> 
> If I understand right, these functions are only called with pad 0 
> (IMGU_NODE_IN). I would drop the pad argument here and use IMGU_NODE_IN. 
> Otherwise it gives a false idea that other pads could be used with these 
> functions, and that would fail for the V4L2_SUBDEV_FORMAT_ACTIVE case.

It thought the same when proposing this. I kept the pad argument as the
driver should ideally be ported to the active state API, which will
provide storage for rectangles on all pads separately. Preparing for
that may not be worth it though, given that the code will probably to be
changed significantly anyway. I'm fine either way.

> However, that's not a big issue. With or without the change:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

-- 
Regards,

Laurent Pinchart