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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 Apr 2019 12:08:36 +0800
From:   Yang Yingliang <yangyingliang@...wei.com>
To:     Paul Moore <paul@...l-moore.com>
CC:     Casey Schaufler <casey@...aufler-ca.com>,
        Oleg Nesterov <oleg@...hat.com>, <john.johansen@...onical.com>,
        "chengjian (D)" <cj.chengjian@...wei.com>,
        Kees Cook <keescook@...omium.org>, NeilBrown <neilb@...e.com>,
        Anna Schumaker <Anna.Schumaker@...app.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        "Xiexiuqi (Xie XiuQi)" <xiexiuqi@...wei.com>,
        Li Bin <huawei.libin@...wei.com>,
        "Jason Yan" <yanaijie@...wei.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Linux Security Module list 
        <linux-security-module@...r.kernel.org>,
        SELinux <selinux@...r.kernel.org>, <yangyingliang@...wei.com>
Subject: Re: kernel BUG at kernel/cred.c:434!



On 2019/4/23 3:48, Paul Moore wrote:
> On Sat, Apr 20, 2019 at 3:39 AM Yang Yingliang <yangyingliang@...wei.com> wrote:
>> I'm not sure you got my point.
> I went back and looked at your previous emails again to try and
> understand what you are talking about, and I'm a little confused by
> some of the output ...
>
>> --- a/kernel/acct.c
>> +++ b/kernel/acct.c
>> @@ -481,6 +481,7 @@ static void do_acct_process(struct bsd_acct_struct
>> *acct)
>>           flim = current->signal->rlim[RLIMIT_FSIZE].rlim_cur;
>>           current->signal->rlim[RLIMIT_FSIZE].rlim_cur = RLIM_INFINITY;
>>           /* Perform file operations on behalf of whoever enabled
>> accounting */
>> +       pr_info("task:%px new cred:%px real cred:%px cred:%px\n",
>> current, file->f_cred, current->real_cred, current->cred);
>>           orig_cred = override_creds(file->f_cred);
> Okay, with this patch applied we should the task/cred info when
> do_acct_process is called.  Got it.
>
>> Messages:
>> [   56.643298] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
>> cred:ffff88841ae450c0 cred:ffff88841ae450c0    //They are same.
> Okay, it looks like do_acct_process() was called and f_cred,
> real_cred, and cred are all the same.
This is a original message, without patch applied.
>
>> [   56.646609] Process accounting resumed
> It looks like do_acct_process() has called check_free_space() now.  So
> far so good.
>
>> [   56.649943] task:ffff88841a9595c0 new cred:ffff88841ae450c0 real
>> cred:ffff88841c96c300 cred:ffff88841ae450c0
> Wait a minute ... why are we seeing this again?  Looking at the task
> pointer and the timestamp, this is the same task exiting and trying to
> write to the accounting file, yes?  This output is particularly
> curious since it appears that real_cred has changed; where is this
> happening?
This is the message when the BUG_ON was triggered without applying any
fix patch.


If we apply this patch "proc: prevent changes to overridden 
credentials", program
runs like this:

1. As print message shows, before overriden, the pointer has the 
following value:
     real_cread=cred=0xffff88841ae450c0, f_cred=0xffff88841ae450c0
     override_creds() is called in do_acct_process():
     ...
     /* Perform file operations on behalf of whoever enabled accounting */
     orig_cred = override_creds(file->f_cred);
     ...


2. After override_creds(), if (current_cred() != current_real_cred()) is 
not work here,
we will call commit_creds()  in security_setprocattr().
     ...
     /* Prevent changes to overridden credentials. */
         if (current_cred() != current_real_cred()) {
             rcu_read_unlock();
         return -EBUSY;
     }
     ...


3. After commit_creds(), we have new cred and real_cred.
     security_setprocattr()    //commit_creds is called here

4. revert_creds() is called in in do_acct_process(), the cred
is reverted to the old value(0xffff88841ae450c0)
     ...
     current->signal->rlim[RLIMIT_FSIZE].rlim_cur = flim;
     revert_creds(orig_cred);

5. After reverting, cred and real_cred are not equal.
If it has a risk to trigger the BUG_ON, when doing another
commit_creds() ?