linux-kernel - Re: [PATCH] ipu3-imgu: Fix NULL pointer dereference in imgu_subdev_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c0ad65c5-818d-da5c-178a-dfaf685f8d24@ideasonboard.com>
Date:   Thu, 8 Sep 2022 15:08:27 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Maximilian Luz <luzmaximilian@...il.com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Bingbu Cao <bingbu.cao@...el.com>,
        Tianshu Qiu <tian.shu.qiu@...el.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        linux-media@...r.kernel.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] ipu3-imgu: Fix NULL pointer dereference in
 imgu_subdev_set_selection()

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@...r.kernel.org # for v5.14 and later
> Signed-off-by: Maximilian Luz <luzmaximilian@...il.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@...asonboard.com>

  Tomi