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

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

[Sodagudi Prasad]

Hi All,

  Would like to understand the reasons behind using the orig_add_limit 
variable in the following code. Can you please share more details ?

"arch/arm64/mm/fault.c"
static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
                                    struct pt_regs *regs)
{
…
…
…
         if (addr < USER_DS && is_permission_fault(esr, regs)) {          
=====>> condition_1
                 /* regs->orig_addr_limit may be 0 if we entered from EL0 
*/
                 if (regs->orig_addr_limit == KERNEL_DS)             	    
=====>> condition_2
                         die("Accessing user space memory with 
fs=KERNEL_DS", regs, esr);

                 if (is_el1_instruction_abort(esr))
                         die("Attempting to execute userspace memory", 
regs, esr);

                 if (!search_exception_tables(regs->pc))
                         die("Accessing user space memory outside 
uaccess.h routines", regs, esr);
         }


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.  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?
2)	Are there any corner cases where these if conditions (condition_1 and 
condition2) would lead to kernel crash ?
3)	What are all scenarios these if conditions (condition_1 and 
condition2)  would like to take care?

-Thanks, Prasad


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

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

[Will Deacon]

On Tue, Feb 14, 2017 at 09:52:30PM -0800, Sodagudi Prasad wrote:
> 
> Hi All,
> 
>  Would like to understand the reasons behind using the orig_add_limit
> variable in the following code. Can you please share more details ?
> 
> "arch/arm64/mm/fault.c"
> static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
>                                    struct pt_regs *regs)
> {
> …
> …
> …
>         if (addr < USER_DS && is_permission_fault(esr, regs)) {
> =====>> condition_1
>                 /* regs->orig_addr_limit may be 0 if we entered from EL0 */
>                 if (regs->orig_addr_limit == KERNEL_DS)             	
> =====>> condition_2
>                         die("Accessing user space memory with fs=KERNEL_DS",
> regs, esr);
> 
>                 if (is_el1_instruction_abort(esr))
>                         die("Attempting to execute userspace memory", regs,
> esr);
> 
>                 if (!search_exception_tables(regs->pc))
>                         die("Accessing user space memory outside uaccess.h
> routines", regs, esr);
>         }
> 
> 
> 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.  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().

Which driver is setting KERNEL_DS prior to accessing userspace and why? It
sounds broken to me. I also don't think it will work with PAN + UAO enabled.

Will

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

[James Morse]

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?


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


> 2)    Are there any corner cases where these if conditions (condition_1 and
> condition2) 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 condition2) 
> 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

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

[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

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

[James Morse]

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?

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


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

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

[Sodagudi Prasad]

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

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

[Laurent Pinchart]

Hi Prasad,

On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
> 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?

It's a bit of historical hack. To implement compat ioctl handling, we copy the 
ioctl 32-bit argument from userspace, turn it into a native 64-bit ioctl 
argument, and call the native ioctl code. That code expects the argument to be 
stored in userspace memory and uses get_user() and put_user() to access it. As 
the 64-bit argument now lives in kernel memory, my understanding is that we 
fake things up with KERNEL_DS.

The ioctl code should be refactored to get rid of this hack.

> 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

-- 
Regards,

Laurent Pinchart

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

[Mauro Carvalho Chehab]

HI Sodagudi,

Em Tue, 21 Feb 2017 06:20:58 -0800
Sodagudi Prasad <psodagud@codeaurora.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

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

[Mauro Carvalho Chehab]

Em Wed, 22 Feb 2017 21:53:08 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Prasad,
> 
> On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
> > 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?  
> 
> It's a bit of historical hack. To implement compat ioctl handling, we copy the 
> ioctl 32-bit argument from userspace, turn it into a native 64-bit ioctl 
> argument, and call the native ioctl code. That code expects the argument to be 
> stored in userspace memory and uses get_user() and put_user() to access it. As 
> the 64-bit argument now lives in kernel memory, my understanding is that we 
> fake things up with KERNEL_DS.

Precisely. Actually, if I remember well, this was needed to pass pointer
arguments from 32 bits userspace to 64 bits kernelspace. There are a lot of
V4L2 ioctls that pass structures with pointers on it. Setting DS cause
those pointers to do the right thing, but yeah, it is hackish.

This used to work fine on x86_64 (when such code was written e. g. Kernel
2.6.1x). I never tested myself on ARM64, but I guess it used to work, as we
received some patches fixing support for some ioctl compat code due to
x86_64/arm64 differences in the past.

On what Kernel version it started to cause troubles? 4.9? If so, then
maybe the breakage is a side effect of VM stack changes.

> The ioctl code should be refactored to get rid of this hack.

Agreed.

Thanks,
Mauro

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

[Laurent Pinchart]

Hi Mauro,

On Wednesday 22 Feb 2017 17:25:41 Mauro Carvalho Chehab wrote:
> Em Wed, 22 Feb 2017 21:53:08 +0200 Laurent Pinchart escreveu:
> > On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
> >> 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?
> > 
> > It's a bit of historical hack. To implement compat ioctl handling, we copy
> > the ioctl 32-bit argument from userspace, turn it into a native 64-bit
> > ioctl argument, and call the native ioctl code. That code expects the
> > argument to be stored in userspace memory and uses get_user() and
> > put_user() to access it. As the 64-bit argument now lives in kernel
> > memory, my understanding is that we fake things up with KERNEL_DS.
> 
> Precisely. Actually, if I remember well, this was needed to pass pointer
> arguments from 32 bits userspace to 64 bits kernelspace. There are a lot of
> V4L2 ioctls that pass structures with pointers on it. Setting DS cause
> those pointers to do the right thing, but yeah, it is hackish.

We should restructure the core ioctl code to decouple copy from/to user and 
ioctl execution (this might just be a matter of exporting a currently static 
function), and change the compat code to perform the copy/from to user 
directly when converting between 32-bit and 64-bit structures (dropping all 
the alloc in userspace hacks) and call the ioctl execution handler. That will 
fix the problem. Any volunteer ? :-)

> This used to work fine on x86_64 (when such code was written e. g. Kernel
> 2.6.1x). I never tested myself on ARM64, but I guess it used to work, as we
> received some patches fixing support for some ioctl compat code due to
> x86_64/arm64 differences in the past.
> 
> On what Kernel version it started to cause troubles? 4.9? If so, then
> maybe the breakage is a side effect of VM stack changes.
> 
> > The ioctl code should be refactored to get rid of this hack.
> 
> Agreed.

-- 
Regards,

Laurent Pinchart