linux-kernel - Re: [PATCH] drm: remove redundant code form drm_ioc32.c

Open Source and information security mailing list archives

Date:	Fri, 03 Jul 2015 14:03:23 +0300
From:	Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:	Jani Nikula <jani.nikula@...ux.intel.com>, airlied@...ux.ie,
	dri-devel@...ts.freedesktop.org
CC:	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm: remove redundant code form drm_ioc32.c


On 07/02/2015 11:10 AM, Jani Nikula wrote:
> On Wed, 01 Jul 2015, Jarkko Sakkinen <jarkko.sakkinen@...ux.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@...ux.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.
Right. Thanks and sorry about sloppiness. I'll update the patch.

>
> 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@...ts.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

/Jarkko
--
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/

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