linux-kernel - Re: [PATCH] drm: mxsfb: check framebuffer pitch

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: <20200908120719.GD6047@pendragon.ideasonboard.com>
Date:   Tue, 8 Sep 2020 15:07:19 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Daniel Vetter <daniel@...ll.ch>
Cc:     Tomi Valkeinen <tomi.valkeinen@...com>,
        Stefan Agner <stefan@...er.ch>, jsarha@...com, marex@...x.de,
        airlied@...ux.ie, shawnguo@...nel.org, s.hauer@...gutronix.de,
        kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
        dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm: mxsfb: check framebuffer pitch

On Tue, Sep 08, 2020 at 10:48:55AM +0200, Daniel Vetter wrote:
> On Tue, Sep 08, 2020 at 11:18:25AM +0300, Tomi Valkeinen wrote:
> > On 08/09/2020 10:55, Stefan Agner wrote:
> > > On 2020-09-07 20:18, Daniel Vetter wrote:
> > >> On Mon, Sep 07, 2020 at 07:17:12PM +0300, Laurent Pinchart wrote:
> > >>> Hi Stefan,
> > >>>
> > >>> Thank you for the patch.
> > >>>
> > >>> On Mon, Sep 07, 2020 at 06:03:43PM +0200, Stefan Agner wrote:
> > >>>> The lcdif IP does not support a framebuffer pitch (stride) other than
> > >>>> the CRTC width. Check for equality and reject the state otherwise.
> > >>>>
> > >>>> This prevents a distorted picture when using 640x800 and running the
> > >>>> Mesa graphics stack. Mesa tires to use a cache aligned stride, which
> > >>>
> > >>> s/tires/tries/
> > >>>
> > >>>> leads at that particular resolution to width != stride. Currently
> > >>>> Mesa has no fallback behavior, but rejecting this configuration allows
> > >>>> userspace to handle the issue correctly.
> > >>>
> > >>> I'm increasingly impressed by how featureful this IP core is :-)
> > >>>
> > >>>> Signed-off-by: Stefan Agner <stefan@...er.ch>
> > >>>> ---
> > >>>>  drivers/gpu/drm/mxsfb/mxsfb_kms.c | 22 ++++++++++++++++++----
> > >>>>  1 file changed, 18 insertions(+), 4 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > >>>> index b721b8b262ce..79aa14027f91 100644
> > >>>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > >>>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
> > >>>> @@ -403,14 +403,28 @@ static int mxsfb_plane_atomic_check(struct drm_plane *plane,
> > >>>>  {
> > >>>>  	struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(plane->dev);
> > >>>>  	struct drm_crtc_state *crtc_state;
> > >>>> +	unsigned int pitch;
> > >>>> +	int ret;
> > >>>>
> > >>>>  	crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
> > >>>>  						   &mxsfb->crtc);
> > >>>>
> > >>>> -	return drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > >>>> -						   DRM_PLANE_HELPER_NO_SCALING,
> > >>>> -						   DRM_PLANE_HELPER_NO_SCALING,
> > >>>> -						   false, true);
> > >>>> +	ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> > >>>> +						  DRM_PLANE_HELPER_NO_SCALING,
> > >>>> +						  DRM_PLANE_HELPER_NO_SCALING,
> > >>>> +						  false, true);
> > >>>> +	if (ret || !plane_state->visible)
> > >>>
> > >>> Would it be more explict to check for !plane_state->fb ? Otherwise I'll
> > >>> have to verify that !fb always implies !visible :-)
> > >>>
> > >>>> +		return ret;
> > >>>> +
> > >>>> +	pitch = crtc_state->mode.hdisplay *
> > >>>> +		plane_state->fb->format->cpp[0];
> > >>>
> > >>> This holds on a single line.
> > >>>
> > >>>> +	if (plane_state->fb->pitches[0] != pitch) {
> > >>>> +		dev_err(plane->dev->dev,
> > >>>> +			"Invalid pitch: fb and crtc widths must be the same");
> > >>>
> > >>> I'd turn this into a dev_dbg(), printing error messages to the kernel
> > >>> log in response to user-triggered conditions is a bit too verbose and
> > >>> could flood the log.
> > >>>
> > >>> Wouldn't it be best to catch this issue when creating the framebuffer ?
> > >>
> > >> Yeah this should be verified at addfb time. We try to validate as early as
> > >> possible.
> > > 
> > > Sounds sensible. From what I can tell fb_create is the proper callback
> > > to implement this at addfb time. Will give this a try.
> > > 
> > > FWIW, I got the idea from drivers/gpu/drm/tilcdc/tilcdc_plane.c. Maybe
> > > should be moved to addfb there too?
> > 
> > But you don't know the crtc width when creating the framebuffer.
> 
> Hm right this is a different check. What we could check in fb_create for
> both is that the logical fb size matches exactly the pitch. That's not
> sufficient criteria, but it will at least catch some of them already.
> 
> But yeah we'd need both here.

Correct. At addfb time we can check the pitch, and at atomic check time
we should check that the plane spans the whole CRTC.

-- 
Regards,

Laurent Pinchart