* [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
@ 2016-10-19 13:59 Leon Yu
2016-10-19 14:13 ` Michal Hocko
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Leon Yu @ 2016-10-19 13:59 UTC (permalink / raw)
To: Andrew Morton, Al Viro, Kees Cook, Oleg Nesterov, Michal Hocko,
John Stultz, Mateusz Guzik, Janis Danisevskis, linux-kernel
Reading auxv of any kernel thread results in NULL pointer dereferencing in
auxv_read() where mm can be NULL or even error code. Fix that by testing mm
with IS_ERR_OR_NULL() helper. This is also the original behavior changed by
recent commit c5317167854e ("proc: switch auxv to use of __mem_open()").
/ # cat /proc/2/auxv
[ 8.964445] Unable to handle kernel NULL pointer dereference at virtual address 000000a8
[ 8.972555] pgd = e99e0000
[ 8.975282] [000000a8] *pgd=399e6835, *pte=00000000, *ppte=00000000
[ 8.981572] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
[ 8.986967] Modules linked in:
[ 8.990029] CPU: 3 PID: 113 Comm: cat Not tainted 4.9.0-rc1-ARCH+ #1
[ 8.996379] Hardware name: BCM2709
[ 8.999778] task: ea3b0b00 task.stack: e99b2000
[ 9.004314] PC is at auxv_read+0x24/0x4c
[ 9.008241] LR is at do_readv_writev+0x2fc/0x37c
[ 9.012860] pc : [<b0135f80>] lr : [<b00e5900>] psr: 80070013
[ 9.012860] sp : e99b3d08 ip : 00000000 fp : 00000000
[ 9.024337] r10: 00000000 r9 : b0135f5c r8 : e99b3d24
[ 9.029561] r7 : 00001000 r6 : 00000000 r5 : 00000000 r4 : 00000000
[ 9.036087] r3 : 00000008 r2 : e99b3de0 r1 : 00001000 r0 : e98ea000
[ 9.042615] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 9.049750] Control: 10c5387d Table: 399e006a DAC: 00000055
[ 9.055495] Process cat (pid: 113, stack limit = 0xe99b2210)
[ 9.061152] Stack: (0xe99b3d08 to 0xe99b4000)
[ 9.065513] 3d00: 00000000 00000000 ea0d46c0 b00e5900 e99b3d20 e99b3d24
[ 9.073696] 3d20: e9960b80 00000002 00000000 00010000 e9960b80 00000010 00000000 024200ca
[ 9.081878] 3d40: ea8855f0 00000000 e9cac8f0 00000000 ffffffff e9960c00 0000000f 00001000
[ 9.090060] 3d60: b0407b68 024200c0 b05494c0 00010000 e9960c00 b020863c 01000000 e99b3ddc
[ 9.098243] 3d80: e99b3dd8 e99651c0 e9960c00 00000000 00010000 b020aca0 e9c041e8 00000010
[ 9.106424] 3da0: e99b3e00 ffffe000 00000000 e99b3ec0 e99b3e00 b00e59c0 e99b3de0 00000000
[ 9.114606] 3dc0: e9960c00 b010eed4 00000000 00020000 ea0d46c0 af000000 e99651c0 00000000
[ 9.122788] 3de0: 00000000 00000000 e99b2000 00000008 00000000 01000000 e9960c00 00000000
[ 9.130970] 3e00: e98ea000 00001000 e9970000 00001000 e9975000 00001000 e9971000 00001000
[ 9.139152] 3e20: e99e5000 00001000 e98e8000 00001000 e9976000 00001000 e9977000 00001000
[ 9.147334] 3e40: e9978000 00001000 e9979000 00001000 e997a000 00001000 e997b000 00001000
[ 9.155516] 3e60: e9a03000 00001000 e99d6000 00001000 e99d7000 00001000 e9a14000 00001000
[ 9.163698] 3e80: 01000000 e9960c00 e99b3ef0 01000000 00000000 ea0d46c0 00000000 00000000
[ 9.171880] 3ea0: 00000000 b010eb4c 00000000 e98f2700 00000817 00000000 b010e268 00000000
[ 9.180061] 3ec0: 00000000 00000000 00000000 e9964480 01000000 e99b3f50 e99b3f48 ea0d46c0
[ 9.188244] 3ee0: 00000000 e99b3f50 01000000 b010ed6c 01000000 01000000 00000000 e9964480
[ 9.196425] 3f00: 00000000 00000000 e99b3f50 00000000 00000000 00000000 00000400 ea0d46c0
[ 9.204607] 3f20: ea0d46c0 7fffffff 00000000 e9964480 e9964480 b00e5e38 01000000 00000000
[ 9.212788] 3f40: 00000000 ea0f21b8 00000000 00000000 00000000 00000000 ea0d46c8 00000000
[ 9.220970] 3f60: 00000000 01000000 000000ef b000f4a4 e99b2000 00000000 00000000 b00e67d4
[ 9.229152] 3f80: 7fffffff 00000000 e99b2000 00000000 01000000 00000000 01000000 000000ef
[ 9.237334] 3fa0: b000f4a4 b000f300 01000000 00000000 00000001 00000003 00000000 01000000
[ 9.245515] 3fc0: 01000000 00000000 01000000 000000ef 00000001 00000000 00000001 00000000
[ 9.253698] 3fe0: a6ee73c0 aeeaecf4 00014bbc a6ee73cc 60070010 00000001 55555555 55555555
[ 9.261895] [<b0135f80>] (auxv_read) from [<b00e5900>] (do_readv_writev+0x2fc/0x37c)
[ 9.269651] [<b00e5900>] (do_readv_writev) from [<b00e59c0>] (vfs_readv+0x40/0x58)
[ 9.277234] [<b00e59c0>] (vfs_readv) from [<b010eed4>] (default_file_splice_read+0x140/0x240)
[ 9.285769] [<b010eed4>] (default_file_splice_read) from [<b010eb4c>] (splice_direct_to_actor+0xa0/0x230)
[ 9.295345] [<b010eb4c>] (splice_direct_to_actor) from [<b010ed6c>] (do_splice_direct+0x90/0xb8)
[ 9.304140] [<b010ed6c>] (do_splice_direct) from [<b00e5e38>] (do_sendfile+0x1a0/0x308)
[ 9.312155] [<b00e5e38>] (do_sendfile) from [<b00e67d4>] (SyS_sendfile64+0xd4/0xe8)
[ 9.319823] [<b00e67d4>] (SyS_sendfile64) from [<b000f300>] (ret_fast_syscall+0x0/0x34)
[ 9.327829] Code: e1a01002 e1a02003 e1a03004 e2833008 (e593e0a0)
[ 9.333973] ---[ end trace d3f50081f24b99ce ]---
Fixes: c5317167854e ("proc: switch auxv to use of __mem_open()")
Signed-off-by: Leon Yu <chianglungyu@gmail.com>
---
fs/proc/base.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index c2964d890c9a..598d08936a3c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1007,6 +1007,9 @@ static ssize_t auxv_read(struct file *file, char __user *buf,
{
struct mm_struct *mm = file->private_data;
unsigned int nwords = 0;
+
+ if (IS_ERR_OR_NULL(mm))
+ return PTR_ERR(mm);
do {
nwords += 2;
} while (mm->saved_auxv[nwords - 2] != 0); /* AT_NULL */
--
2.10.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 13:59 [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv Leon Yu
@ 2016-10-19 14:13 ` Michal Hocko
2016-10-19 16:12 ` Oleg Nesterov
2016-10-19 14:20 ` Al Viro
` (2 subsequent siblings)
3 siblings, 1 reply; 16+ messages in thread
From: Michal Hocko @ 2016-10-19 14:13 UTC (permalink / raw)
To: Leon Yu
Cc: Andrew Morton, Al Viro, Kees Cook, Oleg Nesterov, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On Wed 19-10-16 21:59:40, Leon Yu wrote:
[...]
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c2964d890c9a..598d08936a3c 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1007,6 +1007,9 @@ static ssize_t auxv_read(struct file *file, char __user *buf,
> {
> struct mm_struct *mm = file->private_data;
> unsigned int nwords = 0;
> +
> + if (IS_ERR_OR_NULL(mm))
> + return PTR_ERR(mm);
> do {
> nwords += 2;
> } while (mm->saved_auxv[nwords - 2] != 0); /* AT_NULL */
Why would we even want to open that file if there is no mm struct?
Wouldn't something like this be better? ESRCH might be a bit unexpected
for an existing pid but I am not sure what would be a better error code.
---
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5ef2ae81f623..d34d33dbf1b2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -804,10 +804,10 @@ static const struct file_operations proc_single_file_operations = {
struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
{
struct task_struct *task = get_proc_task(inode);
- struct mm_struct *mm = ERR_PTR(-ESRCH);
+ struct mm_struct *ret = ERR_PTR(-ESRCH);
if (task) {
- mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
+ struct mm_struct *mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
put_task_struct(task);
if (!IS_ERR_OR_NULL(mm)) {
@@ -815,10 +815,11 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
atomic_inc(&mm->mm_count);
/* but do not pin its memory */
mmput(mm);
+ ret = mm;
}
}
- return mm;
+ return ret;
}
static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 13:59 [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv Leon Yu
2016-10-19 14:13 ` Michal Hocko
@ 2016-10-19 14:20 ` Al Viro
2016-10-19 14:51 ` Michal Hocko
2016-10-19 14:51 ` Leon Yu
2016-10-19 17:17 ` Michal Hocko
2016-10-20 12:23 ` [PATCH v2] " Leon Yu
3 siblings, 2 replies; 16+ messages in thread
From: Al Viro @ 2016-10-19 14:20 UTC (permalink / raw)
To: Leon Yu
Cc: Andrew Morton, Kees Cook, Oleg Nesterov, Michal Hocko,
John Stultz, Mateusz Guzik, Janis Danisevskis, linux-kernel
On Wed, Oct 19, 2016 at 09:59:40PM +0800, Leon Yu wrote:
> Reading auxv of any kernel thread results in NULL pointer dereferencing in
> auxv_read() where mm can be NULL or even error code. Fix that by testing mm
> with IS_ERR_OR_NULL() helper. This is also the original behavior changed by
> recent commit c5317167854e ("proc: switch auxv to use of __mem_open()").
What the... How can it be ERR_PTR(...) after it has passed __mem_open()?
I agree that we ought to check for NULL mm (the only question is whether it's
best done by failing open() or by treating the file as empty), but this
IS_ERR_OR_NULL() is pure cargo-cult, AFAICS.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 14:20 ` Al Viro
@ 2016-10-19 14:51 ` Michal Hocko
2016-10-19 14:51 ` Leon Yu
1 sibling, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2016-10-19 14:51 UTC (permalink / raw)
To: Al Viro
Cc: Leon Yu, Andrew Morton, Kees Cook, Oleg Nesterov, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On Wed 19-10-16 15:20:15, Al Viro wrote:
> On Wed, Oct 19, 2016 at 09:59:40PM +0800, Leon Yu wrote:
> > Reading auxv of any kernel thread results in NULL pointer dereferencing in
> > auxv_read() where mm can be NULL or even error code. Fix that by testing mm
> > with IS_ERR_OR_NULL() helper. This is also the original behavior changed by
> > recent commit c5317167854e ("proc: switch auxv to use of __mem_open()").
>
> What the... How can it be ERR_PTR(...) after it has passed __mem_open()?
> I agree that we ought to check for NULL mm (the only question is whether it's
> best done by failing open() or by treating the file as empty), but this
I believe failing open is a better approach because even if a particualr
thread has hijacked a mm via use_mm then the output we will provide is
misleading at best. See my suggestion
http://lkml.kernel.org/r/20161019141346.GJ7517@dhcp22.suse.cz
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 14:20 ` Al Viro
2016-10-19 14:51 ` Michal Hocko
@ 2016-10-19 14:51 ` Leon Yu
1 sibling, 0 replies; 16+ messages in thread
From: Leon Yu @ 2016-10-19 14:51 UTC (permalink / raw)
To: Al Viro
Cc: Andrew Morton, Kees Cook, Oleg Nesterov, Michal Hocko,
John Stultz, Mateusz Guzik, Janis Danisevskis, linux-kernel
On Wed, Oct 19, 2016 at 10:20 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Oct 19, 2016 at 09:59:40PM +0800, Leon Yu wrote:
> > Reading auxv of any kernel thread results in NULL pointer dereferencing in
> > auxv_read() where mm can be NULL or even error code. Fix that by testing mm
> > with IS_ERR_OR_NULL() helper. This is also the original behavior changed by
> > recent commit c5317167854e ("proc: switch auxv to use of __mem_open()").
>
> What the... How can it be ERR_PTR(...) after it has passed __mem_open()?
Doh.. didn't realize that IS_ERR() case can't happen.
> I agree that we ought to check for NULL mm (the only question is whether it's
> best done by failing open() or by treating the file as empty), but this
> IS_ERR_OR_NULL() is pure cargo-cult, AFAICS.
Treating the file in question as empty might be a better choice, just in case
something depends on old behavior.
-Leon
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 14:13 ` Michal Hocko
@ 2016-10-19 16:12 ` Oleg Nesterov
2016-10-19 16:17 ` Michal Hocko
0 siblings, 1 reply; 16+ messages in thread
From: Oleg Nesterov @ 2016-10-19 16:12 UTC (permalink / raw)
To: Michal Hocko
Cc: Leon Yu, Andrew Morton, Al Viro, Kees Cook, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On 10/19, Michal Hocko wrote:
>
> Why would we even want to open that file if there is no mm struct?
Agreed this is strange, but I am not sure we can change this old
behaviour. Say, cat /proc/$pid-of-kthread/mem doesn't fail, it shows
the "empty mm".
Oleg.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 16:12 ` Oleg Nesterov
@ 2016-10-19 16:17 ` Michal Hocko
2016-10-19 16:44 ` Oleg Nesterov
0 siblings, 1 reply; 16+ messages in thread
From: Michal Hocko @ 2016-10-19 16:17 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Leon Yu, Andrew Morton, Al Viro, Kees Cook, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On Wed 19-10-16 18:12:08, Oleg Nesterov wrote:
> On 10/19, Michal Hocko wrote:
> >
> > Why would we even want to open that file if there is no mm struct?
>
> Agreed this is strange, but I am not sure we can change this old
> behaviour. Say, cat /proc/$pid-of-kthread/mem doesn't fail, it shows
> the "empty mm".
What kind of application would break? Anything except for
/proc/self/$file has to handle ESRCH already.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 16:17 ` Michal Hocko
@ 2016-10-19 16:44 ` Oleg Nesterov
2016-10-19 17:00 ` Michal Hocko
0 siblings, 1 reply; 16+ messages in thread
From: Oleg Nesterov @ 2016-10-19 16:44 UTC (permalink / raw)
To: Michal Hocko
Cc: Leon Yu, Andrew Morton, Al Viro, Kees Cook, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On 10/19, Michal Hocko wrote:
>
> On Wed 19-10-16 18:12:08, Oleg Nesterov wrote:
> > On 10/19, Michal Hocko wrote:
> > >
> > > Why would we even want to open that file if there is no mm struct?
> >
> > Agreed this is strange, but I am not sure we can change this old
> > behaviour. Say, cat /proc/$pid-of-kthread/mem doesn't fail, it shows
> > the "empty mm".
>
> What kind of application would break?
I have no idea, probably nobody will ever notice this change, just
I am always nervous when it comes to user-visible changes.
But why do we want this change? Not that I am going to argue if you
submit a patch, but personally I see no real point.
Oleg.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 16:44 ` Oleg Nesterov
@ 2016-10-19 17:00 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2016-10-19 17:00 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Leon Yu, Andrew Morton, Al Viro, Kees Cook, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On Wed 19-10-16 18:44:29, Oleg Nesterov wrote:
> On 10/19, Michal Hocko wrote:
> >
> > On Wed 19-10-16 18:12:08, Oleg Nesterov wrote:
> > > On 10/19, Michal Hocko wrote:
> > > >
> > > > Why would we even want to open that file if there is no mm struct?
> > >
> > > Agreed this is strange, but I am not sure we can change this old
> > > behaviour. Say, cat /proc/$pid-of-kthread/mem doesn't fail, it shows
> > > the "empty mm".
> >
> > What kind of application would break?
>
> I have no idea, probably nobody will ever notice this change, just
> I am always nervous when it comes to user-visible changes.
>
> But why do we want this change? Not that I am going to argue if you
> submit a patch, but personally I see no real point.
I find it much easier to not allow open than a) have a weird side
effects like seeing a hijacked mm and b) risk that similar NULL ptr will
repeat again because all the code paths have to check for it explicitly.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 13:59 [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv Leon Yu
2016-10-19 14:13 ` Michal Hocko
2016-10-19 14:20 ` Al Viro
@ 2016-10-19 17:17 ` Michal Hocko
2016-10-20 12:32 ` Leon Yu
2016-10-20 12:23 ` [PATCH v2] " Leon Yu
3 siblings, 1 reply; 16+ messages in thread
From: Michal Hocko @ 2016-10-19 17:17 UTC (permalink / raw)
To: Leon Yu
Cc: Andrew Morton, Al Viro, Kees Cook, Oleg Nesterov, John Stultz,
Mateusz Guzik, Michael S. Tsirkin, Janis Danisevskis,
linux-kernel
So here is my RFC as an alternative. Thoughts? Please note that we
currently have only very few users of use_mm() API in the kernel
so a risk of a regression is not really high. usb/gadget are using it
only temporarily. The remaining is vhost which operates on a remote mm
and I have no idea whether somebody might abuse /proc/vhost/mem or
anything - let's add Michael to the CC list. I am pretty sure nobody
abuse oom_reaper proc directory as this one is pretty new and such a
usage would be pretty much undefined as the reaper unmaps the address
space.
---
>From 6a1a9fca2871ada365b465382a3f89a1506c312d Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 19 Oct 2016 19:02:25 +0200
Subject: [PATCH] proc: do not allow to open file requiring mm for kernel
threads
Leon Yu has reported the following NULL ptr oops
$ cat /proc/2/auxv
[ 8.964445] Unable to handle kernel NULL pointer dereference at virtual address 000000a8
[ 8.972555] pgd = e99e0000
[ 8.975282] [000000a8] *pgd=399e6835, *pte=00000000, *ppte=00000000
[ 8.981572] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
[ 8.986967] Modules linked in:
[ 8.990029] CPU: 3 PID: 113 Comm: cat Not tainted 4.9.0-rc1-ARCH+ #1
[ 8.996379] Hardware name: BCM2709
[ 8.999778] task: ea3b0b00 task.stack: e99b2000
[ 9.004314] PC is at auxv_read+0x24/0x4c
[ 9.008241] LR is at do_readv_writev+0x2fc/0x37c
[...]
[ 9.261895] [<b0135f80>] (auxv_read) from [<b00e5900>] (do_readv_writev+0x2fc/0x37c)
[ 9.269651] [<b00e5900>] (do_readv_writev) from [<b00e59c0>] (vfs_readv+0x40/0x58)
[ 9.277234] [<b00e59c0>] (vfs_readv) from [<b010eed4>] (default_file_splice_read+0x140/0x240)
[ 9.285769] [<b010eed4>] (default_file_splice_read) from [<b010eb4c>] (splice_direct_to_actor+0xa0/0x230)
[ 9.295345] [<b010eb4c>] (splice_direct_to_actor) from [<b010ed6c>] (do_splice_direct+0x90/0xb8)
[ 9.304140] [<b010ed6c>] (do_splice_direct) from [<b00e5e38>] (do_sendfile+0x1a0/0x308)
[ 9.319823] [<b00e67d4>] (SyS_sendfile64) from [<b000f300>] (ret_fast_syscall+0x0/0x34)
[ 9.327829] Code: e1a01002 e1a02003 e1a03004 e2833008 (e593e0a0)
[ 9.333973] ---[ end trace d3f50081f24b99ce ]---
This has been introduced by c5317167854e ("proc: switch auxv to use
of __mem_open()") but it shows a deeper problem we have had for a
longer time. __mem_open resp. proc_mem_open allows to open a file which
requires the address space even when there is none. This means that all
kernel code paths which use __mem_open have to check for mm!=NULL. This
is error prone as the above shows and also doesn't make much sense in
general. A task doesn't have an mm even when it is already exiting or
when it is a kernel thread. The later will not have an mm unless it
hijacks it from another task when the output might be really misleading
without a deeper knowledge of the particular kernel thread.
Chane proc_mem_open to disallow opening a file if the mm is NULL and
return ESRCH. This is an user visible change theoretically but every
user other than /proc/self/$file has to cope with ESRCH already and
relying on kthread giving any output is just an abuse of the interface.
This will make users f proc_mem_open less error prone as well.
Fixes: c5317167854e ("proc: switch auxv to use of __mem_open()")
Reported-by: Leon Yu <chianglungyu@gmail.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
fs/proc/base.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5ef2ae81f623..d34d33dbf1b2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -804,10 +804,10 @@ static const struct file_operations proc_single_file_operations = {
struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
{
struct task_struct *task = get_proc_task(inode);
- struct mm_struct *mm = ERR_PTR(-ESRCH);
+ struct mm_struct *ret = ERR_PTR(-ESRCH);
if (task) {
- mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
+ struct mm_struct *mm = mm_access(task, mode | PTRACE_MODE_FSCREDS);
put_task_struct(task);
if (!IS_ERR_OR_NULL(mm)) {
@@ -815,10 +815,11 @@ struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode)
atomic_inc(&mm->mm_count);
/* but do not pin its memory */
mmput(mm);
+ ret = mm;
}
}
- return mm;
+ return ret;
}
static int __mem_open(struct inode *inode, struct file *file, unsigned int mode)
--
2.9.3
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 13:59 [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv Leon Yu
` (2 preceding siblings ...)
2016-10-19 17:17 ` Michal Hocko
@ 2016-10-20 12:23 ` Leon Yu
2016-10-20 17:04 ` Oleg Nesterov
3 siblings, 1 reply; 16+ messages in thread
From: Leon Yu @ 2016-10-20 12:23 UTC (permalink / raw)
To: Andrew Morton, Al Viro, Kees Cook, Michal Hocko, Oleg Nesterov,
John Stultz, Mateusz Guzik, Janis Danisevskis, linux-kernel
Reading auxv of any kernel thread results in NULL pointer dereferencing in
auxv_read() where mm can be NULL. Fix that by checking for NULL mm and
bailing out early. This is also the original behavior changed by recent
commit c5317167854e ("proc: switch auxv to use of __mem_open()").
/ # cat /proc/2/auxv
[ 8.964445] Unable to handle kernel NULL pointer dereference at virtual address 000000a8
[ 8.972555] pgd = e99e0000
[ 8.975282] [000000a8] *pgd=399e6835, *pte=00000000, *ppte=00000000
[ 8.981572] Internal error: Oops: 17 [#1] PREEMPT SMP ARM
[ 8.986967] Modules linked in:
[ 8.990029] CPU: 3 PID: 113 Comm: cat Not tainted 4.9.0-rc1-ARCH+ #1
[ 8.996379] Hardware name: BCM2709
[ 8.999778] task: ea3b0b00 task.stack: e99b2000
[ 9.004314] PC is at auxv_read+0x24/0x4c
[ 9.008241] LR is at do_readv_writev+0x2fc/0x37c
[ 9.012860] pc : [<b0135f80>] lr : [<b00e5900>] psr: 80070013
[ 9.012860] sp : e99b3d08 ip : 00000000 fp : 00000000
[ 9.024337] r10: 00000000 r9 : b0135f5c r8 : e99b3d24
[ 9.029561] r7 : 00001000 r6 : 00000000 r5 : 00000000 r4 : 00000000
[ 9.036087] r3 : 00000008 r2 : e99b3de0 r1 : 00001000 r0 : e98ea000
[ 9.042615] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 9.049750] Control: 10c5387d Table: 399e006a DAC: 00000055
[ 9.055495] Process cat (pid: 113, stack limit = 0xe99b2210)
[ 9.061152] Stack: (0xe99b3d08 to 0xe99b4000)
[ 9.065513] 3d00: 00000000 00000000 ea0d46c0 b00e5900 e99b3d20 e99b3d24
[ 9.073696] 3d20: e9960b80 00000002 00000000 00010000 e9960b80 00000010 00000000 024200ca
[ 9.081878] 3d40: ea8855f0 00000000 e9cac8f0 00000000 ffffffff e9960c00 0000000f 00001000
[ 9.090060] 3d60: b0407b68 024200c0 b05494c0 00010000 e9960c00 b020863c 01000000 e99b3ddc
[ 9.098243] 3d80: e99b3dd8 e99651c0 e9960c00 00000000 00010000 b020aca0 e9c041e8 00000010
[ 9.106424] 3da0: e99b3e00 ffffe000 00000000 e99b3ec0 e99b3e00 b00e59c0 e99b3de0 00000000
[ 9.114606] 3dc0: e9960c00 b010eed4 00000000 00020000 ea0d46c0 af000000 e99651c0 00000000
[ 9.122788] 3de0: 00000000 00000000 e99b2000 00000008 00000000 01000000 e9960c00 00000000
[ 9.130970] 3e00: e98ea000 00001000 e9970000 00001000 e9975000 00001000 e9971000 00001000
[ 9.139152] 3e20: e99e5000 00001000 e98e8000 00001000 e9976000 00001000 e9977000 00001000
[ 9.147334] 3e40: e9978000 00001000 e9979000 00001000 e997a000 00001000 e997b000 00001000
[ 9.155516] 3e60: e9a03000 00001000 e99d6000 00001000 e99d7000 00001000 e9a14000 00001000
[ 9.163698] 3e80: 01000000 e9960c00 e99b3ef0 01000000 00000000 ea0d46c0 00000000 00000000
[ 9.171880] 3ea0: 00000000 b010eb4c 00000000 e98f2700 00000817 00000000 b010e268 00000000
[ 9.180061] 3ec0: 00000000 00000000 00000000 e9964480 01000000 e99b3f50 e99b3f48 ea0d46c0
[ 9.188244] 3ee0: 00000000 e99b3f50 01000000 b010ed6c 01000000 01000000 00000000 e9964480
[ 9.196425] 3f00: 00000000 00000000 e99b3f50 00000000 00000000 00000000 00000400 ea0d46c0
[ 9.204607] 3f20: ea0d46c0 7fffffff 00000000 e9964480 e9964480 b00e5e38 01000000 00000000
[ 9.212788] 3f40: 00000000 ea0f21b8 00000000 00000000 00000000 00000000 ea0d46c8 00000000
[ 9.220970] 3f60: 00000000 01000000 000000ef b000f4a4 e99b2000 00000000 00000000 b00e67d4
[ 9.229152] 3f80: 7fffffff 00000000 e99b2000 00000000 01000000 00000000 01000000 000000ef
[ 9.237334] 3fa0: b000f4a4 b000f300 01000000 00000000 00000001 00000003 00000000 01000000
[ 9.245515] 3fc0: 01000000 00000000 01000000 000000ef 00000001 00000000 00000001 00000000
[ 9.253698] 3fe0: a6ee73c0 aeeaecf4 00014bbc a6ee73cc 60070010 00000001 55555555 55555555
[ 9.261895] [<b0135f80>] (auxv_read) from [<b00e5900>] (do_readv_writev+0x2fc/0x37c)
[ 9.269651] [<b00e5900>] (do_readv_writev) from [<b00e59c0>] (vfs_readv+0x40/0x58)
[ 9.277234] [<b00e59c0>] (vfs_readv) from [<b010eed4>] (default_file_splice_read+0x140/0x240)
[ 9.285769] [<b010eed4>] (default_file_splice_read) from [<b010eb4c>] (splice_direct_to_actor+0xa0/0x230)
[ 9.295345] [<b010eb4c>] (splice_direct_to_actor) from [<b010ed6c>] (do_splice_direct+0x90/0xb8)
[ 9.304140] [<b010ed6c>] (do_splice_direct) from [<b00e5e38>] (do_sendfile+0x1a0/0x308)
[ 9.312155] [<b00e5e38>] (do_sendfile) from [<b00e67d4>] (SyS_sendfile64+0xd4/0xe8)
[ 9.319823] [<b00e67d4>] (SyS_sendfile64) from [<b000f300>] (ret_fast_syscall+0x0/0x34)
[ 9.327829] Code: e1a01002 e1a02003 e1a03004 e2833008 (e593e0a0)
[ 9.333973] ---[ end trace d3f50081f24b99ce ]---
Fixes: c5317167854e ("proc: switch auxv to use of __mem_open()")
Signed-off-by: Leon Yu <chianglungyu@gmail.com>
---
Changes in v2:
- remove the bogus test for IS_ERR() case which can't happen after
passing __mem_open().
fs/proc/base.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 8e654468ab67..0baf7f8a5d2d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1014,6 +1014,9 @@ static ssize_t auxv_read(struct file *file, char __user *buf,
{
struct mm_struct *mm = file->private_data;
unsigned int nwords = 0;
+
+ if (!mm)
+ return 0;
do {
nwords += 2;
} while (mm->saved_auxv[nwords - 2] != 0); /* AT_NULL */
--
2.10.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-19 17:17 ` Michal Hocko
@ 2016-10-20 12:32 ` Leon Yu
2016-10-20 12:44 ` Michal Hocko
0 siblings, 1 reply; 16+ messages in thread
From: Leon Yu @ 2016-10-20 12:32 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrew Morton, Al Viro, Kees Cook, Oleg Nesterov, John Stultz,
Mateusz Guzik, Michael S. Tsirkin, Janis Danisevskis,
linux-kernel
On Thu, Oct 20, 2016 at 1:17 AM, Michal Hocko <mhocko@kernel.org> wrote:
> So here is my RFC as an alternative. Thoughts? Please note that we
> currently have only very few users of use_mm() API in the kernel
> so a risk of a regression is not really high. usb/gadget are using it
> only temporarily. The remaining is vhost which operates on a remote mm
> and I have no idea whether somebody might abuse /proc/vhost/mem or
> anything - let's add Michael to the CC list. I am pretty sure nobody
> abuse oom_reaper proc directory as this one is pretty new and such a
> usage would be pretty much undefined as the reaper unmaps the address
> space.
With this patch I cannot tell the difference between a) the thread is
exiting and b) it's a kernel thread,
besides, getting "no such process" while the kthread does exist is a
bit confusing.
IMO, reading /proc/<kthread_pid>/auxv and getting empty output are
quite straightforward,
it doesn't seem to be that "abusive".
-Leon
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-20 12:32 ` Leon Yu
@ 2016-10-20 12:44 ` Michal Hocko
0 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2016-10-20 12:44 UTC (permalink / raw)
To: Leon Yu
Cc: Andrew Morton, Al Viro, Kees Cook, Oleg Nesterov, John Stultz,
Mateusz Guzik, Michael S. Tsirkin, Janis Danisevskis,
linux-kernel
On Thu 20-10-16 20:32:43, Leon Yu wrote:
> On Thu, Oct 20, 2016 at 1:17 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > So here is my RFC as an alternative. Thoughts? Please note that we
> > currently have only very few users of use_mm() API in the kernel
> > so a risk of a regression is not really high. usb/gadget are using it
> > only temporarily. The remaining is vhost which operates on a remote mm
> > and I have no idea whether somebody might abuse /proc/vhost/mem or
> > anything - let's add Michael to the CC list. I am pretty sure nobody
> > abuse oom_reaper proc directory as this one is pretty new and such a
> > usage would be pretty much undefined as the reaper unmaps the address
> > space.
>
> With this patch I cannot tell the difference between a) the thread is
> exiting and b) it's a kernel thread,
> besides, getting "no such process" while the kthread does exist is a
> bit confusing.
Do we really need to distinguish those two cases? In other words under
which conditions something would fail when seeing a pid directory and
ESRCH when opening a file?
> IMO, reading /proc/<kthread_pid>/auxv and getting empty output are
> quite straightforward,
> it doesn't seem to be that "abusive".
Then we need to teach all those implementations to check for kthread
explicitly and return an empty output rather than relying on mm == NULL
because that, as explained, might belong to some process.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-20 12:23 ` [PATCH v2] " Leon Yu
@ 2016-10-20 17:04 ` Oleg Nesterov
2016-10-20 19:21 ` Michal Hocko
0 siblings, 1 reply; 16+ messages in thread
From: Oleg Nesterov @ 2016-10-20 17:04 UTC (permalink / raw)
To: Leon Yu
Cc: Andrew Morton, Al Viro, Kees Cook, Michal Hocko, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On 10/20, Leon Yu wrote:
>
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1014,6 +1014,9 @@ static ssize_t auxv_read(struct file *file, char __user *buf,
> {
> struct mm_struct *mm = file->private_data;
> unsigned int nwords = 0;
> +
> + if (!mm)
> + return 0;
> do {
> nwords += 2;
> } while (mm->saved_auxv[nwords - 2] != 0); /* AT_NULL */
Michal disagrees and I won't argue with his patch which makes __mem_open()
fail if ->mm == NULL. Even if I don't really understand why should we change
the old behaviour, this _can_ break or at least confuse something/someone.
However, even if we do the change above, personally I do think we should
fix the trivial bug first, then surprise the user-space.
Acked-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-20 17:04 ` Oleg Nesterov
@ 2016-10-20 19:21 ` Michal Hocko
2016-10-21 12:47 ` Leon Yu
0 siblings, 1 reply; 16+ messages in thread
From: Michal Hocko @ 2016-10-20 19:21 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Leon Yu, Andrew Morton, Al Viro, Kees Cook, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On Thu 20-10-16 19:04:39, Oleg Nesterov wrote:
> On 10/20, Leon Yu wrote:
> >
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1014,6 +1014,9 @@ static ssize_t auxv_read(struct file *file, char __user *buf,
> > {
> > struct mm_struct *mm = file->private_data;
> > unsigned int nwords = 0;
> > +
> > + if (!mm)
> > + return 0;
> > do {
> > nwords += 2;
> > } while (mm->saved_auxv[nwords - 2] != 0); /* AT_NULL */
>
> Michal disagrees and I won't argue with his patch which makes __mem_open()
> fail if ->mm == NULL. Even if I don't really understand why should we change
> the old behaviour, this _can_ break or at least confuse something/someone.
>
> However, even if we do the change above, personally I do think we should
> fix the trivial bug first, then surprise the user-space.
I am not objecting to this particular patch as it is obviously correct.
I was just hoping to have a more generic solution. The issue was
introduced in this merge window so it is not like we would need as
minimalistic approach possible. If the __mem_open approach sounds too
dangerous I will not pursue it.
Anyway feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>
for this patch.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] proc: fix NULL dereference when reading /proc/<pid>/auxv
2016-10-20 19:21 ` Michal Hocko
@ 2016-10-21 12:47 ` Leon Yu
0 siblings, 0 replies; 16+ messages in thread
From: Leon Yu @ 2016-10-21 12:47 UTC (permalink / raw)
To: Michal Hocko
Cc: Oleg Nesterov, Andrew Morton, Al Viro, Kees Cook, John Stultz,
Mateusz Guzik, Janis Danisevskis, linux-kernel
On Fri, Oct 21, 2016 at 3:21 AM, Michal Hocko <mhocko@kernel.org> wrote:
> On Thu 20-10-16 19:04:39, Oleg Nesterov wrote:
>> On 10/20, Leon Yu wrote:
>> >
>> > --- a/fs/proc/base.c
>> > +++ b/fs/proc/base.c
>> > @@ -1014,6 +1014,9 @@ static ssize_t auxv_read(struct file *file, char __user *buf,
>> > {
>> > struct mm_struct *mm = file->private_data;
>> > unsigned int nwords = 0;
>> > +
>> > + if (!mm)
>> > + return 0;
>> > do {
>> > nwords += 2;
>> > } while (mm->saved_auxv[nwords - 2] != 0); /* AT_NULL */
>>
>> Michal disagrees and I won't argue with his patch which makes __mem_open()
>> fail if ->mm == NULL. Even if I don't really understand why should we change
>> the old behaviour, this _can_ break or at least confuse something/someone.
>>
>> However, even if we do the change above, personally I do think we should
>> fix the trivial bug first, then surprise the user-space.
>
> I am not objecting to this particular patch as it is obviously correct.
> I was just hoping to have a more generic solution. The issue was
> introduced in this merge window so it is not like we would need as
> minimalistic approach possible. If the __mem_open approach sounds too
> dangerous I will not pursue it.
>
> Anyway feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
> for this patch.
Many thanks to all reviewers involved in this discussion for your valueable
time and feedback.
-Leon
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-10-21 12:47 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-19 13:59 [PATCH] proc: fix NULL dereference when reading /proc/<pid>/auxv Leon Yu
2016-10-19 14:13 ` Michal Hocko
2016-10-19 16:12 ` Oleg Nesterov
2016-10-19 16:17 ` Michal Hocko
2016-10-19 16:44 ` Oleg Nesterov
2016-10-19 17:00 ` Michal Hocko
2016-10-19 14:20 ` Al Viro
2016-10-19 14:51 ` Michal Hocko
2016-10-19 14:51 ` Leon Yu
2016-10-19 17:17 ` Michal Hocko
2016-10-20 12:32 ` Leon Yu
2016-10-20 12:44 ` Michal Hocko
2016-10-20 12:23 ` [PATCH v2] " Leon Yu
2016-10-20 17:04 ` Oleg Nesterov
2016-10-20 19:21 ` Michal Hocko
2016-10-21 12:47 ` Leon Yu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.