Re: <Query> Looking more details and reasons for using orig_add_limit.

From: Sodagudi Prasad
Date: Wed Feb 15 2017 - 16:12:17 EST

Hi James and Will,

Thanks James and Will for providing detailed information.
On 2017-02-15 04:09, James Morse wrote:
Hi Prasad,

On 15/02/17 05:52, Sodagudi Prasad wrote:
When any sys call is made from user space orig_addr_limit will be zero and after
that driver is calling set_fs(KERNEL_DS) and then copy_to_user() to user space

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
Currently working on a platform which is ARMv8 based, so disabled ARMv8.1
and ARMv8.2 features (ARM64_PAN, ARM64_HW_AFDBM, LSE_ATOMICS and ARM64_UAO)
on lsk-v4.4-16.09. 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);

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

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)

2) Are there any corner cases where these if conditions (condition_1 and
condition_2) would lead to kernel crash ?

If you do this on behalf of a user space process the kernel will try to clean up
as best it can and carry on. If you access user space from an interrupt handler
or from a kernel thread you can expect the kernel to panic().

3) What are all scenarios these if conditions (condition_1 and condition_2)
would like to take care?

I'm not sure I understand this question. PAN prevents general kernel code from
accessing user space, you have to use the accessors. When you have UAO too, it
can enforce the set_fs() limit as PAN will generate permission faults when the
accessors touch the kernel/user-space after setting the other set_fs() limit.

I hope this helps!



The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project