[PATCH] proc: fix null pointer deref in proc_pid_permission()

[PATCH] proc: fix null pointer deref in proc_pid_permission()

[Xiaotian Feng]

get_proc_task() can fail to search the task and return NULL, put_task_struct()
will then bomb the kernel with following oops:

[ 1870.574045] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
[ 1870.574065] IP: [<ffffffff81217d34>] proc_pid_permission+0x64/0xe0
[ 1870.574088] PGD 112075067 PUD 112814067 PMD 0
[ 1870.574106] Oops: 0002 [#1] PREEMPT SMP

This is a regression introduced by commit 0499680a, kernel should
return -ESRCH if get_proc_task() failed.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vasiliy Kulikov <segoon@openwall.com>
Cc: Stephen Wilson <wilsons@start.ca>
Cc: David Rientjes <rientjes@google.com>
---
 fs/proc/base.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 8173dfd..5485a53 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -654,6 +654,8 @@ static int proc_pid_permission(struct inode *inode, int mask)
 	bool has_perms;
 
 	task = get_proc_task(inode);
+	if (!task)
+		return -ESRCH;
 	has_perms = has_pid_permissions(pid, task, 1);
 	put_task_struct(task);
 
-- 
1.7.5.4

Re: [PATCH] proc: fix null pointer deref in proc_pid_permission()

[Kees Cook]

Hi,

On Wed, Jan 11, 2012 at 01:47:05PM -0500, Xiaotian Feng wrote:
> get_proc_task() can fail to search the task and return NULL, put_task_struct()
> will then bomb the kernel with following oops:
> 
> [ 1870.574045] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> [ 1870.574065] IP: [<ffffffff81217d34>] proc_pid_permission+0x64/0xe0
> [ 1870.574088] PGD 112075067 PUD 112814067 PMD 0
> [ 1870.574106] Oops: 0002 [#1] PREEMPT SMP
> 
> This is a regression introduced by commit 0499680a, kernel should
> return -ESRCH if get_proc_task() failed.

Nice catch!

However since this error is returned to userspace, shouldn't this be
-ENOENT instead?

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH] proc: fix null pointer deref in proc_pid_permission()

[David Rientjes]

On Wed, 11 Jan 2012, Xiaotian Feng wrote:

> get_proc_task() can fail to search the task and return NULL, put_task_struct()
> will then bomb the kernel with following oops:
> 
> [ 1870.574045] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> [ 1870.574065] IP: [<ffffffff81217d34>] proc_pid_permission+0x64/0xe0
> [ 1870.574088] PGD 112075067 PUD 112814067 PMD 0
> [ 1870.574106] Oops: 0002 [#1] PREEMPT SMP
> 
> This is a regression introduced by commit 0499680a, kernel should
> return -ESRCH if get_proc_task() failed.
> 
> Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vasiliy Kulikov <segoon@openwall.com>
> Cc: Stephen Wilson <wilsons@start.ca>
> Cc: David Rientjes <rientjes@google.com>

Acked-by: David Rientjes <rientjes@google.com>

You're right to use -ESRCH so it matches the same behavior as the 
remainder of /proc/pid when get_proc_task() returns NULL.

Re: [PATCH] proc: fix null pointer deref in proc_pid_permission()

[Andrew Morton]

On Wed, 11 Jan 2012 12:43:30 -0800
Kees Cook <keescook@chromium.org> wrote:

> Hi,
> 
> On Wed, Jan 11, 2012 at 01:47:05PM -0500, Xiaotian Feng wrote:
> > get_proc_task() can fail to search the task and return NULL, put_task_struct()
> > will then bomb the kernel with following oops:
> > 
> > [ 1870.574045] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> > [ 1870.574065] IP: [<ffffffff81217d34>] proc_pid_permission+0x64/0xe0
> > [ 1870.574088] PGD 112075067 PUD 112814067 PMD 0
> > [ 1870.574106] Oops: 0002 [#1] PREEMPT SMP
> > 
> > This is a regression introduced by commit 0499680a, kernel should
> > return -ESRCH if get_proc_task() failed.
> 
> Nice catch!
> 
> However since this error is returned to userspace, shouldn't this be
> -ENOENT instead?
> 

Failed get_proc_task() frequently results in -ESRCH.  And less
frequently results in -ENOENT.

It seems odd that inode_operations.permission() would ever return
anything other than zero or -EPERM.

Re: [PATCH] proc: fix null pointer deref in proc_pid_permission()

[Kees Cook]

On Wed, Jan 11, 2012 at 1:58 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 11 Jan 2012 12:43:30 -0800
> Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Jan 11, 2012 at 01:47:05PM -0500, Xiaotian Feng wrote:
>> > get_proc_task() can fail to search the task and return NULL, put_task_struct()
>> > will then bomb the kernel with following oops:
>> >
>> > [ 1870.574045] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
>> > [ 1870.574065] IP: [<ffffffff81217d34>] proc_pid_permission+0x64/0xe0
>> > [ 1870.574088] PGD 112075067 PUD 112814067 PMD 0
>> > [ 1870.574106] Oops: 0002 [#1] PREEMPT SMP
>> >
>> > This is a regression introduced by commit 0499680a, kernel should
>> > return -ESRCH if get_proc_task() failed.
>>
>> Nice catch!
>>
>> However since this error is returned to userspace, shouldn't this be
>> -ENOENT instead?
>
> Failed get_proc_task() frequently results in -ESRCH.  And less
> frequently results in -ENOENT.
>
> It seems odd that inode_operations.permission() would ever return
> anything other than zero or -EPERM.

Right, but won't this show up at ESRCH from open(2)? If this is used
as-is, we just need to have the manpages updated.

-Kees

-- 
Kees Cook
ChromeOS Security

Re: [PATCH] proc: fix null pointer deref in proc_pid_permission()

[Xiaotian Feng]

On Thu, Jan 12, 2012 at 6:41 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jan 11, 2012 at 1:58 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> On Wed, 11 Jan 2012 12:43:30 -0800
>> Kees Cook <keescook@chromium.org> wrote:
>>> On Wed, Jan 11, 2012 at 01:47:05PM -0500, Xiaotian Feng wrote:
>>> > get_proc_task() can fail to search the task and return NULL, put_task_struct()
>>> > will then bomb the kernel with following oops:
>>> >
>>> > [ 1870.574045] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
>>> > [ 1870.574065] IP: [<ffffffff81217d34>] proc_pid_permission+0x64/0xe0
>>> > [ 1870.574088] PGD 112075067 PUD 112814067 PMD 0
>>> > [ 1870.574106] Oops: 0002 [#1] PREEMPT SMP
>>> >
>>> > This is a regression introduced by commit 0499680a, kernel should
>>> > return -ESRCH if get_proc_task() failed.
>>>
>>> Nice catch!
>>>
>>> However since this error is returned to userspace, shouldn't this be
>>> -ENOENT instead?
>>
>> Failed get_proc_task() frequently results in -ESRCH.  And less
>> frequently results in -ENOENT.
>>
>> It seems odd that inode_operations.permission() would ever return
>> anything other than zero or -EPERM.
>
> Right, but won't this show up at ESRCH from open(2)? If this is used
> as-is, we just need to have the manpages updated.
>

You're right, some of get_proc_task() returns -ENOENT.  Maybe we should
return -ENOENT to avoid breaking userspace tools. Andrew?

> -Kees
>
> --
> Kees Cook
> ChromeOS Security

Re: [PATCH] proc: fix null pointer deref in proc_pid_permission()

[Andrew Morton]

On Thu, 12 Jan 2012 10:45:30 +0800 Xiaotian Feng <xtfeng@gmail.com> wrote:

> On Thu, Jan 12, 2012 at 6:41 AM, Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Jan 11, 2012 at 1:58 PM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> >> On Wed, 11 Jan 2012 12:43:30 -0800
> >> Kees Cook <keescook@chromium.org> wrote:
> >>> On Wed, Jan 11, 2012 at 01:47:05PM -0500, Xiaotian Feng wrote:
> >>> > get_proc_task() can fail to search the task and return NULL, put_task_struct()
> >>> > will then bomb the kernel with following oops:
> >>> >
> >>> > [ 1870.574045] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> >>> > [ 1870.574065] IP: [<ffffffff81217d34>] proc_pid_permission+0x64/0xe0
> >>> > [ 1870.574088] PGD 112075067 PUD 112814067 PMD 0
> >>> > [ 1870.574106] Oops: 0002 [#1] PREEMPT SMP
> >>> >
> >>> > This is a regression introduced by commit 0499680a, kernel should
> >>> > return -ESRCH if get_proc_task() failed.
> >>>
> >>> Nice catch!
> >>>
> >>> However since this error is returned to userspace, shouldn't this be
> >>> -ENOENT instead?
> >>
> >> Failed get_proc_task() frequently results in -ESRCH. __And less
> >> frequently results in -ENOENT.
> >>
> >> It seems odd that inode_operations.permission() would ever return
> >> anything other than zero or -EPERM.
> >
> > Right, but won't this show up at ESRCH from open(2)? If this is used
> > as-is, we just need to have the manpages updated.
> >
> 
> You're right, some of get_proc_task() returns -ENOENT.

More of them return -ESRCH.  I lightly checked whether these can be
returned to open() and it seems not.

>  Maybe we should
> return -ENOENT to avoid breaking userspace tools. Andrew?

It's unclear to me that making it ENOENT will fix more than it breaks. 
procfs accesses can return all sorts of things (ENOENT, ENOMEM, ESRCH,
more...) and with "procfs: add hidepid= and gid= mount options" they
can now return different errors, notably EACCESS from
generic_permission() as well as your ESRCH.

We return such a random sprinkle of errors in there that hopefully
we've trained userspace developers to not depend upon any particular
errno!

ESRCH sounds better to me, because it more accurately reflects what
went wrong.  But I'm not very confident in that...