proc: fix null pointer deref in proc_pid_permission()

PATCH In review Xiaotian Feng 4 participants 7 messages Jan 11, 2012 – Jan 12, 2012 Proc filesystem

Share & tools

Xiaotian Feng

author

2012-01-11 10:44 UTC #0 lore raw

PATCH 1/1 proc: fix null pointer deref in proc_pid_permission()

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 <[email protected]>

Cc Al Viro <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Vasiliy Kulikov <[email protected]>

Cc Stephen Wilson <[email protected]>

Cc David Rientjes <[email protected]>

---
 fs/proc/base.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

fs/proc/base.c +2 -0

index 8173dfd..5485a53 100644
@@ -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);

Signature · 1 line

1.7.5.4

Kees Cook

2012-01-11 20:44 UTC #1 lore raw

Hi,

Quoted 10 lines · 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.

Nice catch!

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

-Kees

Signature · 2 lines

Kees Cook
ChromeOS Security

Andrew Morton

2012-01-11 21:58 UTC #2 lore raw

Quoted 11 lines · On Wed, 11 Jan 2012 12:43:30 -0800 Kees Cook <[email protected]> wrote:

Hi,
Quoted 8 lines · 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

Quoted 8 lines

Quoted 2 lines
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.

Kees Cook

2012-01-11 22:41 UTC #3 lore raw

Quoted 16 lines · Andrew Morton

Quoted 14 lines · On Wed, 11 Jan 2012 12:43:30 -0800 Kees Cook <[email protected]> wrote:
Quoted 10 lines · 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.
Nice catch!

Quoted 8 lines

Quoted 2 lines
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

Signature · 2 lines

Kees Cook
ChromeOS Security

Xiaotian Feng

author

2012-01-12 02:45 UTC #4 lore raw

Quoted 21 lines · Kees Cook

Quoted 19 lines · Andrew Morton
Quoted 16 lines · On Wed, 11 Jan 2012 12:43:30 -0800 Kees Cook <[email protected]> wrote:
Quoted 10 lines · 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.
Nice catch!
However since this error is returned to userspace, shouldn't this be -ENOENT instead?

Quoted 9 lines

Quoted 5 lines
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?

Quoted 5 lines

-Kees
--
Kees Cook
ChromeOS Security

Andrew Morton

2012-01-12 03:00 UTC #5 lore raw

Quoted 25 lines · On Thu, 12 Jan 2012 10:45:30 +0800 Xiaotian Feng <[email protected]> wrote:

Quoted 24 lines · Kees Cook
Quoted 22 lines · Andrew Morton
Quoted 9 lines · On Wed, 11 Jan 2012 12:43:30 -0800 Kees Cook <[email protected]> wrote:
Quoted 8 lines · 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
Quoted 7 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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.

Quoted 2 lines

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

David Rientjes

Acked-by on v1 1/1

2012-01-11 21:49 UTC #6 lore raw

In reply to Xiaotian Feng

Quoted 11 lines · 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.

Quoted 6 lines

Signed-off-by Xiaotian Feng <[email protected]>
Cc Al Viro <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Vasiliy Kulikov <[email protected]>
Cc Stephen Wilson <[email protected]>
Cc David Rientjes <[email protected]>

Acked-by David Rientjes <[email protected]>

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