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

[Maximilian Luz <luzmaximilian@gmail.com>]

Hi,

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.

Regards,
Max

---
  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;
  }

--
2.37.3