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

[Bingbu Cao <bingbu.cao@linux.intel.com>]

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@intel.com>



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@vger.kernel.org # for v5.14 and later
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  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