Re: kernel BUG at kernel/cred.c:434!

[chengjian (D)]

On 2019/4/16 11:40, Kees Cook wrote:
> On Mon, Apr 15, 2019 at 11:20 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Mon, Apr 15, 2019 at 11:05 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > > On 04/15, Paul Moore wrote:
> > > > On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > > > > Well, acct("/proc/self/attr/current") doesn't look like a good idea, but I do
> > > > > not know where should we put the additional check... And probably
> > > > > "echo /proc/self/attr/current > /proc/sys/kernel/core_pattern" can hit the
> > > > > same problem, do_coredump() does override_creds() too.
> > > > >
> > > > > May be just add
> > > > >
> > > > >          if (current->cred != current->real_cred)
> > > > >                  return -EACCES;
> > > > >
> > > > > into proc_pid_attr_write(), I dunno.
> > > > Is the problem that do_acct_process() is calling override_creds() and
> > > > the returned/old credentials are being freed before do_acct_process()
> > > > can reinstall the creds via revert_creds()?  Presumably because the
> > > > process accounting is causing the credentials to be replaced?
> > > Afaics, the problem is that do_acct_process() does override_creds() and
> > > then __kernel_write(). Which calls proc_pid_attr_write(), which in turn calls
> > > selinux_setprocattr(), which does another prepare_creds() + commit_creds();
> > > and commit_creds() hits
> > >
> > >          BUG_ON(task->cred != old);
> > Gotcha.  In the process of looking at the backtrace I forgot about the
> > BUG_ON() at the top of the oops message.
> >
> > I wonder what terrible things would happen if we changed the BUG_ON()
> > in commit_creds to simple returning an error an error code to the
> > caller.  There is a warning/requirement in commit_creds() function
> > header comment that it should always return 0.
> Would callers be expected to call abort_creds() on failure? There are
> a number of places where it'd need fixing up. And would likely be best
> with a __must_check marking.
>
> It seems like avoiding the pathological case might be simpler?


Yeah, Avoiding this pathological case is a better solution.

It seems like that we can't commit_creds() during

override_creds() and revert_creds().

So how about just put commit_creds outside !


just like:

 override_creds() // cred -=> new

ÂÂÂ // may BUG_ON if commit_creds done.

ÂÂÂ revert_creds()ÂÂÂÂ //Â cred -=> oldÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 
<-----------|

ÂÂÂ commit_credsÂÂ //Â cred = real_cred = new ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |

[revert_creds]--------------------------------------------------|


[1]--Before we call commit_creds in selinux_setprocattr(),

if we find that cred != real_cred, it may have been overridden

before, we should revert it.

[2]--The same to revert_creds, when we found someone have committed,

orig_cred != current->real_cred may hits, this means that

we have reverted before(see [1]).

[3]--Sometimes new and old are the same, then we need to consider this

situation specially.


The code just like:


From: Yang Yingliang <yangyingliang@xxxxxxxxxx>
Date: Sat, 13 Apr 2019 21:56:01 +0800
Subject: [PATCH] fix cred bug_on

Signed-off-by: Yang Yingliang <yangyingliang@xxxxxxxxxx>
---
Âkernel/acct.cÂÂÂÂÂÂÂÂÂÂÂ | 3 ++-
Âkernel/cred.cÂÂÂÂÂÂÂÂÂÂÂ | 6 ++++++
Âsecurity/selinux/hooks.c | 2 ++
Â3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/acct.c b/kernel/acct.c
index addf7732fb56..f2065f899eee 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -522,7 +522,8 @@ static void do_acct_process(struct bsd_acct_struct 
*acct)
ÂÂÂÂ }
Âout:
ÂÂÂÂ current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
-ÂÂÂ revert_creds(orig_cred);
+ÂÂÂ if (orig_cred == current->real_cred)ÂÂÂ // [2]
+ÂÂÂ ÂÂÂ revert_creds(orig_cred);
Â}

Â/**
diff --git a/kernel/cred.c b/kernel/cred.c
index ecf03657e71c..c4d5ba92fb9b 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -522,6 +522,9 @@ const struct cred *override_creds(const struct cred 
*new)
Â{
ÂÂÂÂ const struct cred *old = current->cred;

+ÂÂÂ if (old == new)ÂÂÂ //Â [3]
+ÂÂÂ ÂÂÂ return old;
+
ÂÂÂÂ kdebug("override_creds(%p{%d,%d})", new,
ÂÂÂÂ ÂÂÂÂÂÂ atomic_read(&new->usage),
ÂÂÂÂ ÂÂÂÂÂÂ read_cred_subscribers(new));
@@ -551,6 +554,9 @@ void revert_creds(const struct cred *old)
Â{
ÂÂÂÂ const struct cred *override = current->cred;

+ÂÂÂ if (override == old)ÂÂÂ // [3]
+ÂÂÂ ÂÂÂ return;
+
ÂÂÂÂ kdebug("revert_creds(%p{%d,%d})", old,
ÂÂÂÂ ÂÂÂÂÂÂ atomic_read(&old->usage),
ÂÂÂÂ ÂÂÂÂÂÂ read_cred_subscribers(old));
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b5017beb4ef7..bc8108e4e90f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6590,6 +6590,8 @@ static int selinux_setprocattr(const char *name, 
void *value, size_t size)
ÂÂÂÂ ÂÂÂ goto abort_change;
ÂÂÂÂ }

+ÂÂÂ if (current->cred != current->real_cred)ÂÂÂ // [1]
+ÂÂÂ ÂÂÂ revert_creds(current->real_cred);
ÂÂÂÂ commit_creds(new);
ÂÂÂÂ return size;

--
2.17.1


We have tested this patch for 3 days and it works well.

Are there any cases that are not covered here ?


Thanks.

ÂÂÂ Cheng Jian