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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 74F75C4332F for ; Tue, 12 Oct 2021 14:13:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5684B60EFE for ; Tue, 12 Oct 2021 14:13:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230195AbhJLOPW (ORCPT ); Tue, 12 Oct 2021 10:15:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56074 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236953AbhJLOPV (ORCPT ); Tue, 12 Oct 2021 10:15:21 -0400 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 49F7CC06161C for ; Tue, 12 Oct 2021 07:13:19 -0700 (PDT) Received: by mail-ed1-x52a.google.com with SMTP id z20so27305edc.13 for ; Tue, 12 Oct 2021 07:13:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ieKETngd4wQHVII+IJcs5CQY5wbzV3iEx77J8xRx2vA=; b=bWaI4AeFcuNvWGD1I8cg7W6CG/azm1vaRBxjX1dsRUwDZWLVCDsVzS85MZqppDPqW9 34rds3D76bwkkPcvjsGuCTXawVOxwdB47aCElSGmvamfQoH+DDNnXd3o+dxEIfXWrjbD 9So44VhRb03JVhThMjaEnhw5YCUaZMPe/VCwNVxdjRE0o6eP32scGLvrv27NbXQkVhLG QYRO+iyEJ5oGBWxHSDEfHBIXts79CsmV5cbX2N+FVqkQp/3zRx7G8QI8fCoNxTXhr8Lp cOCeSQ0fT/VSaazPoGa379Fp+1k7ZCgjR7y/PXuIDNZ0PKRb5OjvhAoDrfEbCq60UVDa yVWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ieKETngd4wQHVII+IJcs5CQY5wbzV3iEx77J8xRx2vA=; b=QThb0FXzWK4wSTVbqeAxckTEmDhU9coBsXPbSGYZTzTE2EewLNk+2u4zPKTL8/6WEv Qg1NSJHA4oZcmCXmKQq5shZrrWqxymhWpMRJgozgNs1OFnoIyMRQURTde4Skm9pK/VH+ SabfYiOqt1TYEA7v5fd8vlzUepMJQHFeKEcJCzko/fenFsT+zeRMHlCOtaSeDpE+5GRx qWMyqMHJRqiVs+HwPgG1ICwfTiJce8+9h2w34cS4a0Hs/0lFqniIfHK1Uf+PV2kwmmMe eBqGuIAFesPNdS5NlALSbrY3enAr5pGZmGgoFprQMElC15m9J0EciNwUocS5DcR4T17T nT6g== X-Gm-Message-State: AOAM530L8eAm2RHNjd7fHpErpXV1jsq7X4ThEWCeDd1T0XvoF4lsV54y fTg8pQ03TV6l03YBdy2t8cgfZ1HvaC4Qwcj8PzM1 X-Google-Smtp-Source: ABdhPJwOMKZYa/n4x55N6Emss5rBP+EcS7P39AqLbYHCH7HSMgM0Rumxq877HQtO60UwMW0rF+Cn72qdnnCrQ2cNdok= X-Received: by 2002:a17:906:2f16:: with SMTP id v22mr32291219eji.126.1634047997689; Tue, 12 Oct 2021 07:13:17 -0700 (PDT) MIME-Version: 1.0 References: <20211007004629.1113572-1-tkjos@google.com> <20211007004629.1113572-3-tkjos@google.com> <8c07f9b7-58b8-18b5-84f8-9b6c78acb08b@schaufler-ca.com> <20211012094101.GE8429@kadam> In-Reply-To: <20211012094101.GE8429@kadam> From: Paul Moore Date: Tue, 12 Oct 2021 10:13:06 -0400 Message-ID: Subject: Re: [PATCH v4 2/3] binder: use cred instead of task for getsecid To: Dan Carpenter Cc: Casey Schaufler , Todd Kjos , zohar@linux.ibm.com, arve@android.com, joel@joelfernandes.org, devel@driverdev.osuosl.org, Jeffrey Vander Stoep , James Morris , kernel-team@android.com, tkjos@android.com, keescook@chromium.org, jannh@google.com, selinux@vger.kernel.org, Eric Paris , maco@android.com, christian@brauner.io, gregkh@linuxfoundation.org, Stephen Smalley , linux-kernel@vger.kernel.org, stable@vger.kernel.org, linux-security-module@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 12, 2021 at 5:41 AM Dan Carpenter wrote: > > On Mon, Oct 11, 2021 at 02:59:13PM -0700, Casey Schaufler wrote: > > On 10/11/2021 2:33 PM, Paul Moore wrote: > > > On Wed, Oct 6, 2021 at 8:46 PM Todd Kjos wrote: > > >> Use the 'struct cred' saved at binder_open() to lookup > > >> the security ID via security_cred_getsecid(). This > > >> ensures that the security context that opened binder > > >> is the one used to generate the secctx. > > >> > > >> Fixes: ec74136ded79 ("binder: create node flag to request sender's > > >> security context") > > >> Signed-off-by: Todd Kjos > > >> Suggested-by: Stephen Smalley > > >> Reported-by: kernel test robot > > >> Cc: stable@vger.kernel.org # 5.4+ > > >> --- > > >> v3: added this patch to series > > >> v4: fix build-break for !CONFIG_SECURITY > > >> > > >> drivers/android/binder.c | 11 +---------- > > >> include/linux/security.h | 4 ++++ > > >> 2 files changed, 5 insertions(+), 10 deletions(-) > > >> > > >> diff --git a/drivers/android/binder.c b/drivers/android/binder.c > > >> index ca599ebdea4a..989afd0804ca 100644 > > >> --- a/drivers/android/binder.c > > >> +++ b/drivers/android/binder.c > > >> @@ -2722,16 +2722,7 @@ static void binder_transaction(struct binder_proc *proc, > > >> u32 secid; > > >> size_t added_size; > > >> > > >> - /* > > >> - * Arguably this should be the task's subjective LSM secid but > > >> - * we can't reliably access the subjective creds of a task > > >> - * other than our own so we must use the objective creds, which > > >> - * are safe to access. The downside is that if a task is > > >> - * temporarily overriding it's creds it will not be reflected > > >> - * here; however, it isn't clear that binder would handle that > > >> - * case well anyway. > > >> - */ > > >> - security_task_getsecid_obj(proc->tsk, &secid); > > >> + security_cred_getsecid(proc->cred, &secid); > > >> ret = security_secid_to_secctx(secid, &secctx, &secctx_sz); > > >> if (ret) { > > >> return_error = BR_FAILED_REPLY; > > >> diff --git a/include/linux/security.h b/include/linux/security.h > > >> index 6344d3362df7..f02cc0211b10 100644 > > >> --- a/include/linux/security.h > > >> +++ b/include/linux/security.h > > >> @@ -1041,6 +1041,10 @@ static inline void security_transfer_creds(struct cred *new, > > >> { > > >> } > > >> > > >> +static inline void security_cred_getsecid(const struct cred *c, u32 *secid) > > >> +{ > > >> +} > > > Since security_cred_getsecid() doesn't return an error code we should > > > probably set the secid to 0 in this case, for example: > > > > > > static inline void security_cred_getsecid(...) > > > { > > > *secid = 0; > > > } > > > > If CONFIG_SECURITY is unset there shouldn't be any case where > > the secid value is ever used for anything. Are you suggesting that > > it be set out of an abundance of caution? > > The security_secid_to_secctx() function is probably inlined so probably > KMSan will not warn about this. But Smatch will warn about passing > unitialized variables. You probably wouldn't recieve and email about > it, and I would just add an exception that security_cred_getsecid() > should be ignored. I'd much rather just see the secid set to zero in the !CONFIG_SECURITY case. -- paul moore www.paul-moore.com