Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c0ad65c5-818d-da5c-178a-dfaf685f8d24@ideasonboard.com>
Date:   Thu, 8 Sep 2022 15:08:27 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] ipu3-imgu: Fix NULL pointer dereference in
 imgu_subdev_set_selection()
Content-Language: en-US
To:     Maximilian Luz <luzmaximilian@gmail.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>
Cc:     Bingbu Cao <bingbu.cao@intel.com>,
        Tianshu Qiu <tian.shu.qiu@intel.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
References: <20220907224409.3187482-1-luzmaximilian@gmail.com>
From:   Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
In-Reply-To: <20220907224409.3187482-1-luzmaximilian@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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