HI Sodagudi,

Em Tue, 21 Feb 2017 06:20:58 -0800
Sodagudi Prasad [off-list ref] 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:

quoted

Hi Prasad,

On 15/02/17 21:12, Sodagudi Prasad wrote:  

quoted

On 2017-02-15 04:09, James Morse wrote:  

quoted

On 15/02/17 05:52, Sodagudi Prasad wrote:  

quoted

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  

quoted

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.  

quoted

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().

quoted

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:  

quoted

	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.

quoted

quoted

quoted

quoted

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:  

quoted

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