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

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