mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	airlied@linux.ie, dri-devel@lists.freedesktop.org,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drm: remove redundant code form drm_ioc32.c
Date: Thu, 2 Jul 2015 16:24:18 +0200	[thread overview]
Message-ID: <20150702142418.GC23343@phenom.ffwll.local> (raw)
In-Reply-To: <87lhezugip.fsf@intel.com>

On Thu, Jul 02, 2015 at 11:10:06AM +0300, Jani Nikula wrote:
> On Wed, 01 Jul 2015, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> 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.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  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..77b63e7 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 || !argp)
> 
> The risks of touching old code... who does access_ok on argp now? That
> doesn't come from compat_alloc_user_space.
> 
> Same pattern repeated below a few times.

Oops, good catch. Patch dropped again.
-Daniel

> 
> BR,
> Jani.
> 
> 
> >  		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 || !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 || !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@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

  reply	other threads:[~2015-07-02 14:21 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-01  9:24 Jarkko Sakkinen
2015-07-02  7:32 ` Daniel Vetter
2015-07-03 10:59   ` Jarkko Sakkinen
2015-07-02  8:10 ` Jani Nikula
2015-07-02 14:24   ` Daniel Vetter [this message]
2015-07-03 11:03   ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150702142418.GC23343@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=jani.nikula@linux.intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome