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: <20200908084855.GH2352366@phenom.ffwll.local>
Date:   Tue, 8 Sep 2020 10:48:55 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Tomi Valkeinen <tomi.valkeinen@...com>
Cc:     Stefan Agner <stefan@...er.ch>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        jsarha@...com, marex@...x.de, airlied@...ux.ie, daniel@...ll.ch,
        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 11:18:25AM +0300, Tomi Valkeinen wrote:
> Hi,
> 
> 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.
> >> -Daniel
> >>
> > 
> > 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.
-Daniel

> 
>  Tomi
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ