* [PATCH] proc: fix null pointer deref in proc_pid_permission() @ 2012-01-11 18:47 Xiaotian Feng 2012-01-11 20:43 ` Kees Cook 2012-01-11 21:49 ` David Rientjes 0 siblings, 2 replies; 7+ messages in thread From: Xiaotian Feng @ 2012-01-11 18:47 UTC (permalink / raw) To: linux-kernel Cc: Xiaotian Feng, Xiaotian Feng, Al Viro, Andrew Morton, Vasiliy Kulikov, Stephen Wilson, David Rientjes 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] proc: fix null pointer deref in proc_pid_permission() 2012-01-11 18:47 [PATCH] proc: fix null pointer deref in proc_pid_permission() Xiaotian Feng @ 2012-01-11 20:43 ` Kees Cook 2012-01-11 21:58 ` Andrew Morton 2012-01-11 21:49 ` David Rientjes 1 sibling, 1 reply; 7+ messages in thread From: Kees Cook @ 2012-01-11 20:43 UTC (permalink / raw) To: Xiaotian Feng Cc: linux-kernel, Xiaotian Feng, Al Viro, Andrew Morton, Vasiliy Kulikov, Stephen Wilson, David Rientjes 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] proc: fix null pointer deref in proc_pid_permission() 2012-01-11 20:43 ` Kees Cook @ 2012-01-11 21:58 ` Andrew Morton 2012-01-11 22:41 ` Kees Cook 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2012-01-11 21:58 UTC (permalink / raw) To: Kees Cook Cc: Xiaotian Feng, linux-kernel, Xiaotian Feng, Al Viro, Vasiliy Kulikov, Stephen Wilson, David Rientjes 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] proc: fix null pointer deref in proc_pid_permission() 2012-01-11 21:58 ` Andrew Morton @ 2012-01-11 22:41 ` Kees Cook 2012-01-12 2:45 ` Xiaotian Feng 0 siblings, 1 reply; 7+ messages in thread From: Kees Cook @ 2012-01-11 22:41 UTC (permalink / raw) To: Andrew Morton Cc: Xiaotian Feng, linux-kernel, Xiaotian Feng, Al Viro, Vasiliy Kulikov, Stephen Wilson, David Rientjes 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] proc: fix null pointer deref in proc_pid_permission() 2012-01-11 22:41 ` Kees Cook @ 2012-01-12 2:45 ` Xiaotian Feng 2012-01-12 3:05 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: Xiaotian Feng @ 2012-01-12 2:45 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, linux-kernel, Xiaotian Feng, Al Viro, Vasiliy Kulikov, Stephen Wilson, David Rientjes 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] proc: fix null pointer deref in proc_pid_permission() 2012-01-12 2:45 ` Xiaotian Feng @ 2012-01-12 3:05 ` Andrew Morton 0 siblings, 0 replies; 7+ messages in thread From: Andrew Morton @ 2012-01-12 3:05 UTC (permalink / raw) To: Xiaotian Feng Cc: Kees Cook, linux-kernel, Xiaotian Feng, Al Viro, Vasiliy Kulikov, Stephen Wilson, David Rientjes 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... ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] proc: fix null pointer deref in proc_pid_permission() 2012-01-11 18:47 [PATCH] proc: fix null pointer deref in proc_pid_permission() Xiaotian Feng 2012-01-11 20:43 ` Kees Cook @ 2012-01-11 21:49 ` David Rientjes 1 sibling, 0 replies; 7+ messages in thread From: David Rientjes @ 2012-01-11 21:49 UTC (permalink / raw) To: Xiaotian Feng Cc: linux-kernel, Xiaotian Feng, Al Viro, Andrew Morton, Vasiliy Kulikov, Stephen Wilson 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-01-12 3:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-01-11 18:47 [PATCH] proc: fix null pointer deref in proc_pid_permission() Xiaotian Feng 2012-01-11 20:43 ` Kees Cook 2012-01-11 21:58 ` Andrew Morton 2012-01-11 22:41 ` Kees Cook 2012-01-12 2:45 ` Xiaotian Feng 2012-01-12 3:05 ` Andrew Morton 2012-01-11 21:49 ` David Rientjes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox