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

[psodagud@codeaurora.org (Sodagudi Prasad)]

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

> 
> 
>> 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!
> 
> 
> Thanks,
> 
> James

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