From patchwork Mon Apr 16 14:50:27 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauro Carvalho Chehab X-Patchwork-Id: 10343167 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B56C06039A for ; Mon, 16 Apr 2018 14:50:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A570F286B5 for ; Mon, 16 Apr 2018 14:50:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 998C028701; Mon, 16 Apr 2018 14:50:39 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.9 required=2.0 tests=BAYES_00, MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id AE3E5286B5 for ; Mon, 16 Apr 2018 14:50:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752060AbeDPOug (ORCPT ); Mon, 16 Apr 2018 10:50:36 -0400 Received: from osg.samsung.com ([64.30.133.232]:50317 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750884AbeDPOue (ORCPT ); Mon, 16 Apr 2018 10:50:34 -0400 Received: from localhost (localhost [127.0.0.1]) by osg.samsung.com (Postfix) with ESMTP id CECCE25AA4; Mon, 16 Apr 2018 07:50:33 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at dev.s-opensource.com Received: from osg.samsung.com ([127.0.0.1]) by localhost (localhost [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id WYhboEFTqjAo; Mon, 16 Apr 2018 07:50:32 -0700 (PDT) Received: from vento.lan (unknown [201.86.164.1]) by osg.samsung.com (Postfix) with ESMTPSA id 7495625A9B; Mon, 16 Apr 2018 07:50:30 -0700 (PDT) Date: Mon, 16 Apr 2018 11:50:27 -0300 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: Linux Media Mailing List , Mauro Carvalho Chehab , Hans Verkuil , Sakari Ailus , Daniel Mentz , Laurent Pinchart Subject: Re: [PATCHv2 17/17] media: v4l2-compat-ioctl32: fix several __user annotations Message-ID: <20180416115027.2fbca161@vento.lan> In-Reply-To: <5c0bb6fe-3148-b5ad-7b78-2a425369c48d@xs4all.nl> References: <55ced09a79ad9947c73187bfbcf85fac220a6d27.1523546545.git.mchehab@s-opensource.com> <5c0bb6fe-3148-b5ad-7b78-2a425369c48d@xs4all.nl> Organization: Samsung X-Mailer: Claws Mail 3.15.1-dirty (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Em Mon, 16 Apr 2018 14:03:45 +0200 Hans Verkuil escreveu: > On 04/13/2018 08:07 PM, Mauro Carvalho Chehab wrote: > > Smatch report several issues with bad __user annotations: > > > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces) > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: expected void [noderef] *uptr > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: got void * > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces) > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: expected void const volatile [noderef] * > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: got struct v4l2_plane [noderef] ** > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces) > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: expected void [noderef] *uptr > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: got void *[assigned] base > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces) > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: expected struct v4l2_ext_control [noderef] *kcontrols > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: got struct v4l2_ext_control * > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces) > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: expected unsigned char [usertype] *__pu_val > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: got void [noderef] * > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces) > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: expected void [noderef] *uptr > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: got void *[assigned] edid > > > > Fix them. > > > > Signed-off-by: Mauro Carvalho Chehab > > --- > > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 32 ++++++++++++++------------- > > 1 file changed, 17 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > index d03a44d89649..0b9dfe7dbfe7 100644 > > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c > > @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up, > > return -EFAULT; > > break; > > case V4L2_MEMORY_USERPTR: > > - if (get_user(p, &up->m.userptr) || > > - put_user((compat_ulong_t)ptr_to_compat((__force void *)p), > > + if (get_user(p, &up->m.userptr)|| > > + put_user((compat_ulong_t)ptr_to_compat((void __user *)p), > > &up32->m.userptr)) > > return -EFAULT; > > break; > > @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp, > > u32 length; > > enum v4l2_memory memory; > > struct v4l2_plane32 __user *uplane32; > > - struct v4l2_plane __user *uplane; > > + struct v4l2_plane *uplane; > > This needs a comment (either here or before the get_user below). It really is a > pointer to userspace, but since videodev2.h has it without __user (since it is > copied to kernel space in v4l2-ioctl.c) we need to define it as a regular pointer > here and cast it to a __user pointer in the put_v4l2_plane32() call. > > This is not trivially obvious, so a comment would help a lot. > > > compat_caddr_t p; > > int ret; > > > > @@ -617,15 +617,14 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp, > > > > if (num_planes == 0) > > return 0; > > - > > - if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes))) > > + if (get_user(uplane, &kp->m.planes)) > > return -EFAULT; > > if (get_user(p, &up->m.planes)) > > return -EFAULT; > > uplane32 = compat_ptr(p); > > > > while (num_planes--) { > > - ret = put_v4l2_plane32(uplane, uplane32, memory); > > + ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory); > > if (ret) > > return ret; > > ++uplane; > > @@ -675,7 +674,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp, > > > > if (!access_ok(VERIFY_READ, up, sizeof(*up)) || > > get_user(tmp, &up->base) || > > - put_user((__force void *)compat_ptr(tmp), &kp->base) || > > + put_user((void __force *)compat_ptr(tmp), &kp->base) || > > assign_in_user(&kp->capability, &up->capability) || > > assign_in_user(&kp->flags, &up->flags) || > > copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt))) > > @@ -690,7 +689,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp, > > > > if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) || > > get_user(base, &kp->base) || > > - put_user(ptr_to_compat(base), &up->base) || > > + put_user(ptr_to_compat((void __user *)base), &up->base) || > > assign_in_user(&up->capability, &kp->capability) || > > assign_in_user(&up->flags, &kp->flags) || > > copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt))) > > @@ -857,7 +856,7 @@ static int put_v4l2_ext_controls32(struct file *file, > > struct v4l2_ext_controls32 __user *up) > > { > > struct v4l2_ext_control32 __user *ucontrols; > > - struct v4l2_ext_control __user *kcontrols; > > + struct v4l2_ext_control *kcontrols; > > u32 count; > > u32 n; > > compat_caddr_t p; > > @@ -883,10 +882,12 @@ static int put_v4l2_ext_controls32(struct file *file, > > unsigned int size = sizeof(*ucontrols); > > u32 id; > > > > - if (get_user(id, &kcontrols->id) || > > + if (get_user(id, (unsigned int __user *)&kcontrols->id) || > > put_user(id, &ucontrols->id) || > > - assign_in_user(&ucontrols->size, &kcontrols->size) || > > - copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2, > > + assign_in_user(&ucontrols->size, > > + (unsigned int __user *)&kcontrols->size) || > > + copy_in_user(&ucontrols->reserved2, > > + (unsigned int __user *)&kcontrols->reserved2, > > sizeof(ucontrols->reserved2))) > > return -EFAULT; > > > > @@ -898,7 +899,8 @@ static int put_v4l2_ext_controls32(struct file *file, > > if (ctrl_is_pointer(file, id)) > > size -= sizeof(ucontrols->value64); > > > > - if (copy_in_user(ucontrols, kcontrols, size)) > > + if (copy_in_user(ucontrols, > > + (unsigned int __user *)kcontrols, size)) > > This is rather ugly. Would it be better to do something like this: > > struct v4l2_ext_control __user *kcontrols > struct v4l2_ext_control *kcontrols_tmp; > > get_user(kcontrols_tmp, &kp->controls); > kcontrols = (void __user __force *)kcontrols_tmp; Nah, having two pointers with the same value, one with __user and another one without it will make it really hard for people to review. > > And then there is no need to change anything else. > > Regardless of the chosen solution, this needs comments to explain what is going > on here, just as with v4l2_buffer above. Actually, the problem here happens before that. The problem here (and on the previous one) is that get_user() expects that the second argument to be a Kernelspace pointer. So, in this routine, it does (simplified): struct v4l2_ext_control32 __user *ucontrols; struct v4l2_ext_control *kcontrols; ... get_user(kcontrols, &kp->controls); Then, every time it needs to get something related to it, it needs the __user, like here: if (get_user(id, (unsigned int __user *)&kcontrols->id) ... In this specific case, only copy_in_user() was missing it; all the other __user casts are there already. > Note: the whole 'u' and 'k' naming is now hopelessly out of date and > confusing. It should really be '32' and '64' to denote 32 bit vs > 64 bit layout. The pointers are now always in userspace, so 'k' no longer > makes sense. Yes, we need this change, but this should be at a separate patch. I can do it, after we cleanup v4l2-compat-ioctl32.c from their namespace mess. > > > return -EFAULT; > > > > ucontrols++; > > @@ -954,7 +956,7 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp, > > assign_in_user(&kp->start_block, &up->start_block) || > > assign_in_user(&kp->blocks, &up->blocks) || > > get_user(tmp, &up->edid) || > > - put_user(compat_ptr(tmp), &kp->edid) || > > + put_user((void __force *)compat_ptr(tmp), &kp->edid) || > > copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved))) > > return -EFAULT; > > return 0; > > @@ -970,7 +972,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp, > > assign_in_user(&up->start_block, &kp->start_block) || > > assign_in_user(&up->blocks, &kp->blocks) || > > get_user(edid, &kp->edid) || > > - put_user(ptr_to_compat(edid), &up->edid) || > > + put_user(ptr_to_compat((void __user *)edid), &up->edid) || > > copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved))) > > return -EFAULT; > > return 0; > > > > Otherwise this patch looks good. > > Regards, > > Hans Would be ok if I fold (or add as a separate patch) the enclosed diff? Thanks, Mauro diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index 1057ab8ce2b6..5c3408bdfd89 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -617,6 +617,15 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp, if (num_planes == 0) return 0; + /* + * We needed to define uplane without user. + * The reason is that v4l2-ioctl.c copies it from userspace + * into Kernelspace, so it's definition at videodev2.h doesn't + * have an __user markup. That makes get_user() to do wrong + * casts, as pointed by smatch. + * So, instead, declare it as ks, and pass it as an userspace + * pointer to put_v4l2_plane32(). + */ if (get_user(uplane, &kp->m.planes)) return -EFAULT; if (get_user(p, &up->m.planes)) @@ -861,6 +870,15 @@ static int put_v4l2_ext_controls32(struct file *file, u32 n; compat_caddr_t p; + /* + * We needed to define kcontrols without user. + * The reason is that v4l2-ioctl.c copies it from userspace + * into Kernelspace, so it's definition at videodev2.h doesn't + * have an __user markup. That makes get_user() & friends to do + * wrong casts, as pointed by smatch. + * So, instead, declare it as ks, and pass it as an userspace + * pointer where needed. + */ if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) || assign_in_user(&up->which, &kp->which) || get_user(count, &kp->count) ||