Sakari,
Thanks for your patch, does it make sense that prevent the
NULL deference and throw warning in v4l2-subdev API? Though it
is the responsibility of driver to keep it safe.
Reviewed-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
On 8/26/22 3:03 AM, 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.
>
> Also rewrap lines a little.
>
> Fixes: 0d346d2a6f54 ("media: v4l2-subdev: add subdev-wide state struct")
> Cc: stable@xxxxxxxxxxxxxxx # for v5.14 and later
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
> drivers/staging/media/ipu3/ipu3-v4l2.c | 31 ++++++++++++--------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index d1c539cefba87..2234bb8d48b34 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -192,33 +192,30 @@ static int imgu_subdev_get_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_selection *sel)
> {
> - struct v4l2_rect *try_sel, *r;
> - struct imgu_v4l2_subdev *imgu_sd = container_of(sd,
> - struct imgu_v4l2_subdev,
> - subdev);
> + struct imgu_v4l2_subdev *imgu_sd =
> + container_of(sd, struct imgu_v4l2_subdev, subdev);
>
> if (sel->pad != IMGU_NODE_IN)
> return -EINVAL;
>
> switch (sel->target) {
> case V4L2_SEL_TGT_CROP:
> - try_sel = v4l2_subdev_get_try_crop(sd, sd_state, sel->pad);
> - r = &imgu_sd->rect.eff;
> - break;
> + 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;
> + return 0;
> case V4L2_SEL_TGT_COMPOSE:
> - try_sel = v4l2_subdev_get_try_compose(sd, sd_state, sel->pad);
> - r = &imgu_sd->rect.bds;
> - break;
> + 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;
> + return 0;
> default:
> return -EINVAL;
> }
> -
> - if (sel->which == V4L2_SUBDEV_FORMAT_TRY)
> - sel->r = *try_sel;
> - else
> - sel->r = *r;
> -
> - return 0;
> }
>
> static int imgu_subdev_set_selection(struct v4l2_subdev *sd,
>
--
Best regards,
Bingbu Cao
