linux-kernel - Re: [PATCH v2 01/14] drm/fb-helper: keep the .gamma_store updated in drm_fb_helper_setcmap

Open Source and information security mailing list archives

Date:   Mon, 26 Jun 2017 11:27:37 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Peter Rosin <peda@...ntia.se>
Cc:     linux-kernel@...r.kernel.org, nouveau@...ts.freedesktop.org,
        dri-devel@...ts.freedesktop.org,
        virtualization@...ts.linux-foundation.org,
        amd-gfx@...ts.freedesktop.org,
        Daniel Vetter <daniel.vetter@...el.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Russell King <linux@...linux.org.uk>,
        Ben Skeggs <bskeggs@...hat.com>,
        Dave Airlie <airlied@...hat.com>,
        intel-gfx@...ts.freedesktop.org,
        Vincent Abriou <vincent.abriou@...com>,
        Philippe Cornu <philippe.cornu@...com>,
        Yannick Fertre <yannick.fertre@...com>,
        Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>,
        Gerd Hoffmann <kraxel@...hat.com>
Subject: Re: [PATCH v2 01/14] drm/fb-helper: keep the .gamma_store updated in
 drm_fb_helper_setcmap

On Thu, Jun 22, 2017 at 08:06:24AM +0200, Peter Rosin wrote:
> I think the gamma_store can end up invalid on error. But the way I read
> it, that can happen in drm_mode_gamma_set_ioctl as well, so why should
> this pesky legacy fbdev stuff be any better?
> 
> Signed-off-by: Peter Rosin <peda@...ntia.se>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 574af01..25315fb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1223,12 +1223,16 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  	const struct drm_crtc_helper_funcs *crtc_funcs;
>  	u16 *red, *green, *blue, *transp;
>  	struct drm_crtc *crtc;
> +	u16 *r, *g, *b;
>  	int i, j, rc = 0;
>  	int start;
>  
>  	if (oops_in_progress)
>  		return -EBUSY;
>  
> +	if (cmap->start + cmap->len < cmap->start)
> +		return -EINVAL;

Doesn't the fbdev core check this for us?

> +
>  	drm_modeset_lock_all(dev);
>  	if (!drm_fb_helper_is_bound(fb_helper)) {
>  		drm_modeset_unlock_all(dev);
> @@ -1245,6 +1249,29 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  		transp = cmap->transp;
>  		start = cmap->start;
>  
> +		if (info->fix.visual != FB_VISUAL_TRUECOLOR) {

I think switching to a positive check of visual == PSEUDOCOLOR is a bit
clearer. Also this makes more sense once we move over to the main
gamma_set function, since as-is it's unclear that setcolreg only does
something for PSEUDOCOLOR. But interim I guess that's ok.
-Daniel

> +			if (!crtc->gamma_size) {
> +				rc = -EINVAL;
> +				goto out;
> +			}
> +
> +			if (cmap->start + cmap->len > crtc->gamma_size) {
> +				rc = -EINVAL;
> +				goto out;
> +			}
> +
> +			r = crtc->gamma_store;
> +			g = r + crtc->gamma_size;
> +			b = g + crtc->gamma_size;
> +
> +			memcpy(r + cmap->start, cmap->red,
> +			       cmap->len * sizeof(u16));
> +			memcpy(g + cmap->start, cmap->green,
> +			       cmap->len * sizeof(u16));
> +			memcpy(b + cmap->start, cmap->blue,
> +			       cmap->len * sizeof(u16));
> +		}
> +
>  		for (j = 0; j < cmap->len; j++) {
>  			u16 hred, hgreen, hblue, htransp = 0xffff;
>  
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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