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
From: Will Deacon <hidden> Date: 2017-02-15 11:38:43
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
From: James Morse <james.morse@arm.com> Date: 2017-02-15 12:09:54
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
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:
quoted
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
quoted
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)
quoted
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().
quoted
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
From: James Morse <james.morse@arm.com> Date: 2017-02-16 10:39:36
Hi Prasad,
On 15/02/17 21:12, Sodagudi Prasad wrote:
On 2017-02-15 04:09, James Morse wrote:
quoted
On 15/02/17 05:52, Sodagudi Prasad wrote:
quoted
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()?
quoted
quoted
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
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:
quoted
On 2017-02-15 04:09, James Morse wrote:
quoted
On 15/02/17 05:52, Sodagudi Prasad wrote:
quoted
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
quoted
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:
quoted
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.
quoted
quoted
quoted
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:
quoted
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
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:
quoted
Hi Prasad,
On 15/02/17 21:12, Sodagudi Prasad wrote:
quoted
On 2017-02-15 04:09, James Morse wrote:
quoted
On 15/02/17 05:52, Sodagudi Prasad wrote:
quoted
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().
quoted
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:
quoted
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.
quoted
quoted
quoted
quoted
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:
quoted
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
HI Sodagudi,
Em Tue, 21 Feb 2017 06:20:58 -0800
Sodagudi Prasad [off-list ref] 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:
quoted
Hi Prasad,
On 15/02/17 21:12, Sodagudi Prasad wrote:
quoted
On 2017-02-15 04:09, James Morse wrote:
quoted
On 15/02/17 05:52, Sodagudi Prasad wrote:
quoted
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
quoted
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.
quoted
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().
quoted
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:
quoted
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.
quoted
quoted
quoted
quoted
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:
quoted
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
Em Wed, 22 Feb 2017 21:53:08 +0200
Laurent Pinchart [off-list ref] escreveu:
Hi Prasad,
On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
quoted
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.
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:
quoted
On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
quoted
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.
quoted
The ioctl code should be refactored to get rid of this hack.