Re: [PATCH 1/1] ipu3-imgu: Fix NULL pointer dereference in active selection access

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Max,

On Wed, Sep 07, 2022 at 03:44:44PM +0200, Maximilian Luz wrote:
> On Thu, 25 Aug 2022 22:03:51 +0300, Sakari Ailus  wrote:
> > What the IMGU driver did was that it first acquired the pointers to active
> > and try V4L2 subdev state, and only then figured out which one to use.
> > 
> > The problem with that approach and a later patch (see Fixes: tag) is that
> > as sd_state argument to v4l2_subdev_get_try_crop() et al is NULL, there is
> > now an attempt to dereference that.
> > 
> > Fix this.
> 
> Thank you for this fix, this however only addresses
> imgu_subdev_get_selection(), but the issue is also present in
> imgu_subdev_set_selection(). I assume that a similar fix is needed for that.
> I've added a diff for that below. Feel free to squash that into your patch or
> let me know if I should submit this separately.

This looks like a problem indeed. I'll let Sakari decide what he wants
to do. Adding wrappers along the lines of

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(&imgu_sd->subdev, sd_state, pad);
	else
		return &imgu_sd->rect.eff;
}

(same for the selection rectangle) and using them through the code may
help.

> ---
>   drivers/staging/media/ipu3/ipu3-v4l2.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index 2234bb8d48b3..079f2635c70d 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -223,10 +223,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 +237,22 @@ 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;
> +               if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> +                       rect = v4l2_subdev_get_try_crop(sd, sd_state, sel->pad);
> +               else
> +                       rect = &imgu_sd->rect.eff;
>                  break;
>          case V4L2_SEL_TGT_COMPOSE:
> -               try_sel = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
> -               rect = &imgu_sd->rect.bds;
> +               if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> +                       rect = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
> +               else
> +                       rect = &imgu_sd->rect.bds;
>                  break;
>          default:
>                  return -EINVAL;
>          }
> 
> -       if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> -               *try_sel = sel->r;
> -       else
> -               *rect = sel->r;
> -
> +       *rect = sel->r;
>          return 0;
>   }
> 

-- 
Regards,

Laurent Pinchart