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?
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
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Linux Foundation Collaborative Project
