Date:   Wed, 22 Feb 2017 21:53:08 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Sodagudi Prasad <psodagud@...eaurora.org>
Cc:     James Morse <james.morse@....com>, mchehab@...pensource.com,
        linux-media@...r.kernel.org, shijie.huang@....com,
        catalin.marinas@....com, will.deacon@....com, mark.rutland@....com,
        akpm@...ux-foundation.org, sandeepa.s.prabhu@...il.com,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        hans.verkuil@...co.com, sakari.ailus@...ux.intel.com,
        tiffany.lin@...iatek.com, nick@...anahar.org, shuah@...nel.org,
        ricardo.ribalda@...il.com
Subject: Re: <Query> Looking more details and reasons for using orig_add_limit.

Hi Prasad,

On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
> Hi mchehab/linux-media,
> 
> It is not clear why KERNEL_DS was set explicitly here. In this path
> video_usercopy() gets  called  and it
> copies the “struct v4l2_buffer” struct to user space stack memory.
> 
> Can you please share reasons for setting to KERNEL_DS here?

It's a bit of historical hack. To implement compat ioctl handling, we copy the 
ioctl 32-bit argument from userspace, turn it into a native 64-bit ioctl 
argument, and call the native ioctl code. That code expects the argument to be 
stored in userspace memory and uses get_user() and put_user() to access it. As 
the 64-bit argument now lives in kernel memory, my understanding is that we 
fake things up with KERNEL_DS.

The ioctl code should be refactored to get rid of this hack.

> static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned
> long arg)
> {
> …
> …
> 
>          if (compatible_arg)
>                  err = native_ioctl(file, cmd, (unsigned long)up);
>          else {
>                  mm_segment_t old_fs = get_fs();
> 
>                  set_fs(KERNEL_DS);
>                  err = native_ioctl(file, cmd, (unsigned long)&karg);
>                  set_fs(old_fs);
>          }
> …
> }
> 
> On 2017-02-16 02:39, James Morse wrote:
> > Hi Prasad,
> > 
> > On 15/02/17 21:12, Sodagudi Prasad wrote:
> >> On 2017-02-15 04:09, James Morse wrote:
> >>> On 15/02/17 05:52, Sodagudi Prasad wrote:
> >>>> that driver is calling set_fs(KERNEL_DS) and  then copy_to_user() to
> >>>> user space
> >>>> memory.
> >>> 
> >>> Don't do this, its exactly the case PAN+UAO and the code you pointed
> >>> to are
> >>> designed to catch. Accessing userspace needs doing carefully, setting
> >>> USER_DS
> >>> and using the put_user()/copy_to_user() accessors are the required
> >>> steps.
> >>> 
> >>> Which driver is doing this? Is it in mainline?
> >> 
> >> Yes. It is mainline driver -
> >> drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> >> 
> >> In some v4l2 use-case kernel panic is observed. Below part
> >> of the code has set_fs to KERNEL_DS before calling native_ioctl().
> >> 
> >> static long do_video_ioctl(struct file *file, unsigned int cmd,
> >> unsigned long arg)
> >> {
> >> …
> >> …
> >> 
> >>         if (compatible_arg)
> >>         
> >>                 err = native_ioctl(file, cmd, (unsigned long)up);
> >>         
> >>         else {
> >>         
> >>                 mm_segment_t old_fs = get_fs();
> >>                 
> >>                 set_fs(KERNEL_DS);   ====> KERNEL_DS.
> >>                 err = native_ioctl(file, cmd, (unsigned long)&karg);
> >>                 set_fs(old_fs);
> >>         
> >>         }
> >> 
> >> Here is the call stack which is resulting crash, because user space
> >> memory has
> >> read only permissions.
> >> [27249.920041] [<ffffff8008357890>] __arch_copy_to_user+0x110/0x180
> >> [27249.920047] [<ffffff8008847c98>] video_ioctl2+0x38/0x44
> >> [27249.920054] [<ffffff8008840968>] v4l2_ioctl+0x78/0xb4
> >> [27249.920059] [<ffffff80088542d8>] do_video_ioctl+0x91c/0x1160
> >> [27249.920064] [<ffffff8008854b7c>] v4l2_compat_ioctl32+0x60/0xcc
> >> [27249.920071] [<ffffff800822553c>] compat_SyS_ioctl+0x124/0xd88
> >> [27249.920077] [<ffffff8008084e30>] el0_svc_naked+0x24/0x2
> > 
> > It's not totally clear to me what is going on here, but some
> > observations:
> > the ioctl is trying to copy_to_user() to some read-only memory.  This
> > would
> > normally fail gracefully with -EFAULT, but because KERNEL_DS has been
> > set, the
> > kernel checks this before calling the fault handler and calls die() on
> > your ioctl().
> > 
> > The ioctl code is doing this deliberately as a compat mechanism, but
> > the code
> > behind file->f_op->unlocked_ioctl() expects fs==USER_DS when it does
> > its work.
> > That code needs to be made aware of this compat translation, or a
> > compat_ioctl
> > call provided.
> > 
> > 
> > Which v4l driver is this? Which ioctl is being called? Does the driver
> > using the
> > v4l framework have a compat_ioctl() call?
> 
> Yes. Same kernel crash is seen with both video and camera use cases.
> Yes. Driver have compact_ioctl().
> 
> > What path does this call take through v4l2_compat_ioctl32()? It looks
> > like
> > compat_ioctl will be skipped in certain cases, v4l2_compat_ioctl32()
> > 
> > has:
> >> 	if (_IOC_TYPE(cmd) == 'V' && _IOC_NR(cmd) < BASE_VIDIOC_PRIVATE)
> >> 	
> >> 		ret = do_video_ioctl(file, cmd, arg);
> >> 	
> >> 	else if (vdev->fops->compat_ioctl32)
> >> 	
> >> 		ret = vdev->fops->compat_ioctl32(file, cmd, arg);
> > 
> > Is your ioctl matched by that top if()?
> 
> Yes.  Top if condition in true and do_video_ioctl() getting called.
> 
> >>>> If there is permission fault for user space address the above
> >>>> condition
> >>>> is leading to kernel crash. Because orig_add_limit is having
> >>>> KERNEL_DS as set_fs
> >>>> called before copy_to_user().
> >>>> 
> >>>> 1)    So I would like to understand that,  is that user space
> >>>> pointer leading to
> >>>> permission fault not correct(condition_1) in this scenario?
> >>> 
> >>> The correct thing has happened here. To access user space
> >>> set_fs(USER_DS) first.
> >>> (and set it back to whatever it was afterwards).
> >> 
> >> So, Any clean up needed to above call path similar to what was done in
> >> the below
> >> commit?
> >> commit a7f61e89af73e9bf760826b20dba4e637221fcb9 - compat_ioctl: don't
> >> call
> >> do_ioctl under set_fs(KERNEL_DS)
> > 
> > That's clever. Is that code doing a conversion, or do you have a
> > compat_ioctl()
> > in your driver?
> > 
> > It's possible that fs/compat_ioctl.c has done this work, but
> > do_video_ioctl()
> > un-does it. Someone who knows about v4l and compat-ioctls should take a
> > look...
> > 
> > This looks like a case of:
> >> The accidental invocation of an unlocked_ioctl handler that
> >> unexpectedly
> >> calls copy_to_user could be a severe security issue.
> > 
> > that Jann describes in the commit message. Fixing the code behind
> > file->f_op->unlocked_ioctl() to consider compat calls from
> > do_video_ioctl() is
> > one way to solve this.
> > 
> > 
> > 
> > Thanks,
> > 
> > James
> 
> -Thanks, Prasad

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists