From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D81D0C282DC for ; Wed, 17 Apr 2019 14:30:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A9ACE217D7 for ; Wed, 17 Apr 2019 14:30:24 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=paul-moore-com.20150623.gappssmtp.com header.i=@paul-moore-com.20150623.gappssmtp.com header.b="XqD4anXL" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732408AbfDQOaX (ORCPT ); Wed, 17 Apr 2019 10:30:23 -0400 Received: from mail-lj1-f196.google.com ([209.85.208.196]:40334 "EHLO mail-lj1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731959AbfDQOaW (ORCPT ); Wed, 17 Apr 2019 10:30:22 -0400 Received: by mail-lj1-f196.google.com with SMTP id q66so1244268ljq.7 for ; Wed, 17 Apr 2019 07:30:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RuyB+W2pZ+QYBWd1eUaaacF+UuRg4xdSTEXA3oHZsgI=; b=XqD4anXLFYevMs1lRTyx2zcvCQubqkTY0PZtzr3CqPikpdNRZHVYUdC2GGYDSv5VPg WP7AOjjquBqxlihIVYE8Ypqmhww8GkwTIOFvHBaV1gZbcaUvBqkQgs6CUKIThv7mT9nM O9SiJhcvum1RY7z4u/fPT2Rx+OnWtc6EiEgWzmqnHEHvA+fJ/vJ4UtkcdUfdH01sFnx2 2h5S1k5yplv9wpvVPXuwYWGr/lv6AE6PUhWlmvDk/RYEdzwF7uJ34XrUdoIuJKSoM2IP uKqyh/uuIjalzz3U+m+JazO2dcqKqch99mbK9AEfN143+E4lJE/yu5lmPpEbVWaFuQyw x3/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RuyB+W2pZ+QYBWd1eUaaacF+UuRg4xdSTEXA3oHZsgI=; b=Y9SDs711lCaAnCjxjLQB3J8kCNoF8zT2haYlAugh7NCvvbVJe1A0uMSLy+2L4NiMKo s7llncHIZqM8nJ2fJ6pfNUthOZW/stB+kBUk3I/7efwdrB/SXVNZURHod78JtbhdwdmD e5f5uVbq8I9AW3fFLuvMIPha3TSxA/AIqsmTYJfHDqW0VPlHy3x+0czMtOeaBmqtcvfg 41JahaRyhq68SzwV5Avl+oEeQ1SrUiznK0FCsLrBJ6zgKpS2dD7gs0qoVvV2e9QTLOwM ZNtiNwkYhwsiSGLLSX7O099T/d79oe4HrmGoydUQlDpz4/8sJsQdGqVknbsm++A8mjZa Qe8Q== X-Gm-Message-State: APjAAAX0eDeQ+ockK5jZyxfLrOMhiqh6odONsqeV9eNtDHVBZlMecceS zbtjHDxXVmG57dU9FOvBz5i7dMeYVBDZsjwMqi4C X-Google-Smtp-Source: APXvYqxRX2PGOIrx7dxH9uSGpkYFbI6pJPfeVsyp3u9ILZZQnkg2xJwcGRdaU/hpbceypTXYTq6mSPH0Nbc8CBFjxXQ= X-Received: by 2002:a2e:42cf:: with SMTP id h76mr46177693ljf.95.1555511420086; Wed, 17 Apr 2019 07:30:20 -0700 (PDT) MIME-Version: 1.0 References: <6e4428ca-3da1-a033-08f7-a51e57503989@huawei.com> <20190415134331.GC22204@redhat.com> <20190415150520.GA13257@redhat.com> In-Reply-To: From: Paul Moore Date: Wed, 17 Apr 2019 10:30:08 -0400 Message-ID: Subject: Re: kernel BUG at kernel/cred.c:434! To: "chengjian (D)" Cc: Kees Cook , Oleg Nesterov , Casey Schaufler , NeilBrown , Anna Schumaker , "linux-kernel@vger.kernel.org" , Al Viro , "Xiexiuqi (Xie XiuQi)" , Li Bin , Jason Yan , Peter Zijlstra , Ingo Molnar , Linux Security Module list , SELinux , Yang Yingliang Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 16, 2019 at 10:46 AM chengjian (D) wrote: > On 2019/4/16 11:40, Kees Cook wrote: > > On Mon, Apr 15, 2019 at 11:20 AM Paul Moore wrote: > >> On Mon, Apr 15, 2019 at 11:05 AM Oleg Nesterov wrote: > >>> On 04/15, Paul Moore wrote: > >>>> On Mon, Apr 15, 2019 at 9:43 AM Oleg Nesterov 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. No arguments that this is particularly messed up scenario, I'm just trying to arrive at a solution that isn't too ugly. > From: Yang Yingliang > Date: Sat, 13 Apr 2019 21:56:01 +0800 > Subject: [PATCH] fix cred bug_on > > Signed-off-by: Yang Yingliang > --- > 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; Doing the revert only to then commit the creds seems really ugly to me. I'm tempted to simply return an error in selinux_setprocattr() if the task's credentials are not the same as its real_cred; if we do that I believe we should resolve this problem. The accounting write to the SELinux file in /proc would fail of course, but I think we can all consider that as a positive side-effect. While I don't think this should have a negative impact on anything else, I haven't convinced myself of that just yet. -- paul moore www.paul-moore.com