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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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