Message-ID: <20170222165347.4c4b7dc8@vento.lan>
Date:   Wed, 22 Feb 2017 16:53:47 -0300
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     Sodagudi Prasad <psodagud@...eaurora.org>
Cc:     James Morse <james.morse@....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,
        laurent.pinchart@...asonboard.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 Sodagudi,

Em Tue, 21 Feb 2017 06:20:58 -0800
Sodagudi Prasad <psodagud@...eaurora.org> escreveu:

> 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?

I'm not sure. If I remember well, such code came from the patch that 
moved the V4L2 compat32 code from FS core to V4L2 in the old days.
As it worked fine since 2.6.x, it was assumed to be the right thing
to do, and we never had any reason to change it to something else :-)

As it is now causing troubles on newer Kernels, it needs to be 
converted to the modern practices. Patches are welcomed!

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



Thanks,
Mauro

Powered by blists - more mailing lists