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

[Andrew Morton <akpm@linux-foundation.org>]

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