Message-ID: <20150703115829.GK23343@phenom.ffwll.local>
Date:	Fri, 3 Jul 2015 13:58:29 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:	airlied@...ux.ie, dri-devel@...ts.freedesktop.org,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm: remove redundant code form drm_ioc32.c

On Fri, Jul 03, 2015 at 02:17:29PM +0300, Jarkko Sakkinen wrote:
> The compat ioctl handler ends up calling access_ok() twice: first
> indirectly inside compat_alloc_user_space() and then after returning
> from that function. This patch fixes issue.
> 
> v2: there were three invalid removals of access_ok() that I've fixed.
> Also went through all the changes couple of times and verified that
> access_ok() is only removed when the buffer is allocated with
> compat_alloc_user_space(). My deepest apologies for this kind of
> sloppiness!
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> Reviewed-by: Jani Nikula <jani.nikula@...ux.intel.com>

This is a forged r-b tag it seems, Jani did not hand you a r-b tag in the
previous discussion under the assumption that you'll fix things up. I've
removed it from your patch.

Ack tags you can add if people make vague approving noises imo, but r-b
tag is a pretty clear statement (see the reviewer's statement of
oversight) and forging them isn't a great idea.
-Daniel

> ---
>  drivers/gpu/drm/drm_ioc32.c | 55 +++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioc32.c b/drivers/gpu/drm/drm_ioc32.c
> index aa8bbb4..8dcfa76 100644
> --- a/drivers/gpu/drm/drm_ioc32.c
> +++ b/drivers/gpu/drm/drm_ioc32.c
> @@ -93,7 +93,7 @@ static int compat_drm_version(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	version = compat_alloc_user_space(sizeof(*version));
> -	if (!access_ok(VERIFY_WRITE, version, sizeof(*version)))
> +	if (!version)
>  		return -EFAULT;
>  	if (__put_user(v32.name_len, &version->name_len)
>  	    || __put_user((void __user *)(unsigned long)v32.name,
> @@ -140,7 +140,7 @@ static int compat_drm_getunique(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	u = compat_alloc_user_space(sizeof(*u));
> -	if (!access_ok(VERIFY_WRITE, u, sizeof(*u)))
> +	if (!u)
>  		return -EFAULT;
>  	if (__put_user(uq32.unique_len, &u->unique_len)
>  	    || __put_user((void __user *)(unsigned long)uq32.unique,
> @@ -168,7 +168,7 @@ static int compat_drm_setunique(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	u = compat_alloc_user_space(sizeof(*u));
> -	if (!access_ok(VERIFY_WRITE, u, sizeof(*u)))
> +	if (!u)
>  		return -EFAULT;
>  	if (__put_user(uq32.unique_len, &u->unique_len)
>  	    || __put_user((void __user *)(unsigned long)uq32.unique,
> @@ -200,7 +200,7 @@ static int compat_drm_getmap(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	map = compat_alloc_user_space(sizeof(*map));
> -	if (!access_ok(VERIFY_WRITE, map, sizeof(*map)))
> +	if (!map)
>  		return -EFAULT;
>  	if (__put_user(idx, &map->offset))
>  		return -EFAULT;
> @@ -237,7 +237,7 @@ static int compat_drm_addmap(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	map = compat_alloc_user_space(sizeof(*map));
> -	if (!access_ok(VERIFY_WRITE, map, sizeof(*map)))
> +	if (!map)
>  		return -EFAULT;
>  	if (__put_user(m32.offset, &map->offset)
>  	    || __put_user(m32.size, &map->size)
> @@ -277,7 +277,7 @@ static int compat_drm_rmmap(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	map = compat_alloc_user_space(sizeof(*map));
> -	if (!access_ok(VERIFY_WRITE, map, sizeof(*map)))
> +	if (!map)
>  		return -EFAULT;
>  	if (__put_user((void *)(unsigned long)handle, &map->handle))
>  		return -EFAULT;
> @@ -306,7 +306,7 @@ static int compat_drm_getclient(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	client = compat_alloc_user_space(sizeof(*client));
> -	if (!access_ok(VERIFY_WRITE, client, sizeof(*client)))
> +	if (!client)
>  		return -EFAULT;
>  	if (__put_user(idx, &client->idx))
>  		return -EFAULT;
> @@ -345,7 +345,7 @@ static int compat_drm_getstats(struct file *file, unsigned int cmd,
>  	int i, err;
>  
>  	stats = compat_alloc_user_space(sizeof(*stats));
> -	if (!access_ok(VERIFY_WRITE, stats, sizeof(*stats)))
> +	if (!stats)
>  		return -EFAULT;
>  
>  	err = drm_ioctl(file, DRM_IOCTL_GET_STATS, (unsigned long)stats);
> @@ -382,8 +382,7 @@ static int compat_drm_addbufs(struct file *file, unsigned int cmd,
>  	unsigned long agp_start;
>  
>  	buf = compat_alloc_user_space(sizeof(*buf));
> -	if (!access_ok(VERIFY_WRITE, buf, sizeof(*buf))
> -	    || !access_ok(VERIFY_WRITE, argp, sizeof(*argp)))
> +	if (!buf || !access_ok(VERIFY_WRITE, argp, sizeof(*argp)))
>  		return -EFAULT;
>  
>  	if (__copy_in_user(buf, argp, offsetof(drm_buf_desc32_t, agp_start))
> @@ -414,7 +413,7 @@ static int compat_drm_markbufs(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	buf = compat_alloc_user_space(sizeof(*buf));
> -	if (!access_ok(VERIFY_WRITE, buf, sizeof(*buf)))
> +	if (!buf)
>  		return -EFAULT;
>  
>  	if (__put_user(b32.size, &buf->size)
> @@ -455,7 +454,7 @@ static int compat_drm_infobufs(struct file *file, unsigned int cmd,
>  
>  	nbytes = sizeof(*request) + count * sizeof(struct drm_buf_desc);
>  	request = compat_alloc_user_space(nbytes);
> -	if (!access_ok(VERIFY_WRITE, request, nbytes))
> +	if (!request)
>  		return -EFAULT;
>  	list = (struct drm_buf_desc *) (request + 1);
>  
> @@ -516,7 +515,7 @@ static int compat_drm_mapbufs(struct file *file, unsigned int cmd,
>  		return -EINVAL;
>  	nbytes = sizeof(*request) + count * sizeof(struct drm_buf_pub);
>  	request = compat_alloc_user_space(nbytes);
> -	if (!access_ok(VERIFY_WRITE, request, nbytes))
> +	if (!request)
>  		return -EFAULT;
>  	list = (struct drm_buf_pub *) (request + 1);
>  
> @@ -563,7 +562,7 @@ static int compat_drm_freebufs(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request)))
> +	if (!request)
>  		return -EFAULT;
>  	if (__put_user(req32.count, &request->count)
>  	    || __put_user((int __user *)(unsigned long)req32.list,
> @@ -589,7 +588,7 @@ static int compat_drm_setsareactx(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request)))
> +	if (!request)
>  		return -EFAULT;
>  	if (__put_user(req32.ctx_id, &request->ctx_id)
>  	    || __put_user((void *)(unsigned long)req32.handle,
> @@ -613,7 +612,7 @@ static int compat_drm_getsareactx(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request)))
> +	if (!request)
>  		return -EFAULT;
>  	if (__put_user(ctx_id, &request->ctx_id))
>  		return -EFAULT;
> @@ -646,7 +645,7 @@ static int compat_drm_resctx(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	res = compat_alloc_user_space(sizeof(*res));
> -	if (!access_ok(VERIFY_WRITE, res, sizeof(*res)))
> +	if (!res)
>  		return -EFAULT;
>  	if (__put_user(res32.count, &res->count)
>  	    || __put_user((struct drm_ctx __user *) (unsigned long)res32.contexts,
> @@ -689,7 +688,7 @@ static int compat_drm_dma(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	d = compat_alloc_user_space(sizeof(*d));
> -	if (!access_ok(VERIFY_WRITE, d, sizeof(*d)))
> +	if (!d)
>  		return -EFAULT;
>  
>  	if (__put_user(d32.context, &d->context)
> @@ -764,7 +763,7 @@ static int compat_drm_agp_info(struct file *file, unsigned int cmd,
>  	int err;
>  
>  	info = compat_alloc_user_space(sizeof(*info));
> -	if (!access_ok(VERIFY_WRITE, info, sizeof(*info)))
> +	if (!info)
>  		return -EFAULT;
>  
>  	err = drm_ioctl(file, DRM_IOCTL_AGP_INFO, (unsigned long)info);
> @@ -807,7 +806,7 @@ static int compat_drm_agp_alloc(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
> +	if (!request
>  	    || __put_user(req32.size, &request->size)
>  	    || __put_user(req32.type, &request->type))
>  		return -EFAULT;
> @@ -834,7 +833,7 @@ static int compat_drm_agp_free(struct file *file, unsigned int cmd,
>  	u32 handle;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
> +	if (!request
>  	    || get_user(handle, &argp->handle)
>  	    || __put_user(handle, &request->handle))
>  		return -EFAULT;
> @@ -858,7 +857,7 @@ static int compat_drm_agp_bind(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
> +	if (!request
>  	    || __put_user(req32.handle, &request->handle)
>  	    || __put_user(req32.offset, &request->offset))
>  		return -EFAULT;
> @@ -874,7 +873,7 @@ static int compat_drm_agp_unbind(struct file *file, unsigned int cmd,
>  	u32 handle;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
> +	if (!request
>  	    || get_user(handle, &argp->handle)
>  	    || __put_user(handle, &request->handle))
>  		return -EFAULT;
> @@ -897,8 +896,7 @@ static int compat_drm_sg_alloc(struct file *file, unsigned int cmd,
>  	unsigned long x;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
> -	    || !access_ok(VERIFY_WRITE, argp, sizeof(*argp))
> +	if (!request || !access_ok(VERIFY_WRITE, argp, sizeof(*argp))
>  	    || __get_user(x, &argp->size)
>  	    || __put_user(x, &request->size))
>  		return -EFAULT;
> @@ -923,8 +921,7 @@ static int compat_drm_sg_free(struct file *file, unsigned int cmd,
>  	unsigned long x;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
> -	    || !access_ok(VERIFY_WRITE, argp, sizeof(*argp))
> +	if (!request || !access_ok(VERIFY_WRITE, argp, sizeof(*argp))
>  	    || __get_user(x, &argp->handle)
>  	    || __put_user(x << PAGE_SHIFT, &request->handle))
>  		return -EFAULT;
> @@ -952,7 +949,7 @@ static int compat_drm_update_draw(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request)) ||
> +	if (!request ||
>  	    __put_user(update32.handle, &request->handle) ||
>  	    __put_user(update32.type, &request->type) ||
>  	    __put_user(update32.num, &request->num) ||
> @@ -994,7 +991,7 @@ static int compat_drm_wait_vblank(struct file *file, unsigned int cmd,
>  		return -EFAULT;
>  
>  	request = compat_alloc_user_space(sizeof(*request));
> -	if (!access_ok(VERIFY_WRITE, request, sizeof(*request))
> +	if (!request
>  	    || __put_user(req32.request.type, &request->request.type)
>  	    || __put_user(req32.request.sequence, &request->request.sequence)
>  	    || __put_user(req32.request.signal, &request->request.signal))
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists