| 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
| ||
|
Message-ID: <20150702142418.GC23343@phenom.ffwll.local> Date: Thu, 2 Jul 2015 16:24:18 +0200 From: Daniel Vetter <daniel@...ll.ch> To: Jani Nikula <jani.nikula@...ux.intel.com> Cc: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>, airlied@...ux.ie, dri-devel@...ts.freedesktop.org, open list <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] drm: remove redundant code form drm_ioc32.c On Thu, Jul 02, 2015 at 11:10:06AM +0300, 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. 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@...ts.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > 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