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;
Date:   Thu, 8 Sep 2022 16:47:57 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc:     Maximilian Luz <luzmaximilian@gmail.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        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>,
        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
Subject: Re: [PATCH] ipu3-imgu: Fix NULL pointer dereference in
 imgu_subdev_set_selection()
Message-ID: <YxnyjYldJ5oAhkBp@pendragon.ideasonboard.com>
References: <20220907224409.3187482-1-luzmaximilian@gmail.com>
 <c0ad65c5-818d-da5c-178a-dfaf685f8d24@ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <c0ad65c5-818d-da5c-178a-dfaf685f8d24@ideasonboard.com>
Precedence: bulk

On Thu, Sep 08, 2022 at 03:08:27PM +0300, Tomi Valkeinen wrote:
> 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.

It thought the same when proposing this. I kept the pad argument as the
driver should ideally be ported to the active state API, which will
provide storage for rectangles on all pads separately. Preparing for
that may not be worth it though, given that the code will probably to be
changed significantly anyway. I'm fine either way.

> However, that's not a big issue. With or without the change:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

-- 
Regards,

Laurent Pinchart