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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86988C6FD1D for ; Thu, 30 Mar 2023 23:33:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231565AbjC3XdJ (ORCPT ); Thu, 30 Mar 2023 19:33:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59316 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231351AbjC3XdI (ORCPT ); Thu, 30 Mar 2023 19:33:08 -0400 Received: from mail-yw1-x112f.google.com (mail-yw1-x112f.google.com [IPv6:2607:f8b0:4864:20::112f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2209093FD for ; Thu, 30 Mar 2023 16:33:06 -0700 (PDT) Received: by mail-yw1-x112f.google.com with SMTP id 00721157ae682-54601d90118so228963457b3.12 for ; Thu, 30 Mar 2023 16:33:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=paul-moore.com; s=google; t=1680219185; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=94cHCZl82A9XHBdyuyycOrzigntjL6kSnbJJ+8pRS1I=; b=NE11GM+BTf/CpwCQcIfcVI9o0qRstmxulk194Pff9YhZcy0tnuk7ZQvpOVMGLkupU2 RFsUMuoXtd15daP5ryZm8upTsP+BgSm3SdNW3GO5bmIYndnRlOtfJwfiJX5r/Nvw6UOx t/c7Ip4kHZaF1gb8r3FTaKaLhF4k/pZUJF73ZlyM9JTyt4rStTshwwU/WZunumRIsA4B YBv7bnMdd7tlF0/oNG5YhZ2lvfVG/AHpgeot/r5b9t2fXlyCIXtAyezvdQoLFSUxGjeF PpnvYfbq77h+u4OvzfjEKg/mzVpZufuR9ygDifBygXiG0Gdzm3MQ/7D6QJanR2HNW2Eg Ou+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680219185; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=94cHCZl82A9XHBdyuyycOrzigntjL6kSnbJJ+8pRS1I=; b=Rr0qVP2xbDcsSeStH6afAexZ0gHu5M1hm84gBteD7bWg+fD1d2K2qE9lZmfEirFA22 J4i1SkE0wlMlQn8e4inaX7CwG7FlDDxjEtGlgL4ru+zSznEM9OUGzaHrRDfKA6u7GaKz gGU0+MONpRl9MX0lwSayPA4YhcvvBaNLRsLEeYNeS+EfJ4c9lLUlp3TMlkoPiQ4T/xM1 XB8LBoSqgwot0RISV8CCcAUZRQP6RAZbIhseSWZ/RKzoxLHjlKRiT/dCAHHuWIpzbuwm ODDk7kfg13rGXysaYKpKtHNMRhfT+9zA+VUOSKQt1NZyJFSe1yd8m5UI74AUC6w8d2lb qKbw== X-Gm-Message-State: AAQBX9cN7Rld0kFTqVwvUCDk7Q7lxhmiU8VmCi/7FQ8+v4nBvNSvE6pU VjgmLn6jhlfYVcYzibqYHEyKdM5qf9R6ZfEvKe59 X-Google-Smtp-Source: AKy350b1RGVoSJMOnOcg7Dlzg2s61atlBq/+cwye4xYhjekc2h3Kv8aLYSQgc4ZC5+nT125pSGhat9ufNhuNmhCveTg= X-Received: by 2002:a81:e405:0:b0:544:d5ce:eb33 with SMTP id r5-20020a81e405000000b00544d5ceeb33mr11851343ywl.8.1680219185107; Thu, 30 Mar 2023 16:33:05 -0700 (PDT) MIME-Version: 1.0 References: <20230315224704.2672-1-casey@schaufler-ca.com> <20230315224704.2672-11-casey@schaufler-ca.com> <61d21f68-8e84-ad85-ef20-fced8c8b916d@schaufler-ca.com> In-Reply-To: <61d21f68-8e84-ad85-ef20-fced8c8b916d@schaufler-ca.com> From: Paul Moore Date: Thu, 30 Mar 2023 19:32:54 -0400 Message-ID: Subject: Re: [PATCH v7 10/11] SELinux: Add selfattr hooks To: Casey Schaufler Cc: linux-security-module@vger.kernel.org, jmorris@namei.org, keescook@chromium.org, john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp, stephen.smalley.work@gmail.com, linux-kernel@vger.kernel.org, linux-api@vger.kernel.org, mic@digikod.net, selinux@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-api@vger.kernel.org On Thu, Mar 30, 2023 at 4:55=E2=80=AFPM Casey Schaufler wrote: > On 3/29/2023 6:13 PM, Paul Moore wrote: > > On Wed, Mar 15, 2023 at 6:52=E2=80=AFPM Casey Schaufler wrote: > >> Add hooks for setselfattr and getselfattr. These hooks are not very > >> different from their setprocattr and getprocattr equivalents, and > >> much of the code is shared. > >> > >> Signed-off-by: Casey Schaufler > >> Cc: selinux@vger.kernel.org > >> Cc: Paul Moore > >> --- > >> security/selinux/hooks.c | 147 +++++++++++++++++++++++++++++++-------= - > >> 1 file changed, 117 insertions(+), 30 deletions(-) > >> > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> index 9403aee75981..8896edf80aa9 100644 > >> --- a/security/selinux/hooks.c > >> +++ b/security/selinux/hooks.c > >> @@ -6348,8 +6348,7 @@ static void selinux_d_instantiate(struct dentry = *dentry, struct inode *inode) > >> inode_doinit_with_dentry(inode, dentry); > >> } > >> > >> -static int selinux_getprocattr(struct task_struct *p, > >> - const char *name, char **value) > >> +static int do_getattr(unsigned int attr, struct task_struct *p, char = **value) > > Are you ready for more naming nitpicks? ;) > > I would expect nothing less. :) > > > Let's call this 'selinux_lsm_getattr()', and the matching setter > > should be 'selinux_lsm_setattr()'. > > As you wish. It's your LSM. > > > >> { > >> const struct task_security_struct *__tsec; > >> u32 sid; > >> @@ -6367,20 +6366,27 @@ static int selinux_getprocattr(struct task_str= uct *p, > >> goto bad; > >> } > >> > >> - if (!strcmp(name, "current")) > >> + switch (attr) { > >> + case LSM_ATTR_CURRENT: > >> sid =3D __tsec->sid; > >> - else if (!strcmp(name, "prev")) > >> + break; > >> + case LSM_ATTR_PREV: > >> sid =3D __tsec->osid; > >> - else if (!strcmp(name, "exec")) > >> + break; > >> + case LSM_ATTR_EXEC: > >> sid =3D __tsec->exec_sid; > >> - else if (!strcmp(name, "fscreate")) > >> + break; > >> + case LSM_ATTR_FSCREATE: > >> sid =3D __tsec->create_sid; > >> - else if (!strcmp(name, "keycreate")) > >> + break; > >> + case LSM_ATTR_KEYCREATE: > >> sid =3D __tsec->keycreate_sid; > >> - else if (!strcmp(name, "sockcreate")) > >> + break; > >> + case LSM_ATTR_SOCKCREATE: > >> sid =3D __tsec->sockcreate_sid; > >> - else { > >> - error =3D -EINVAL; > >> + break; > >> + default: > >> + error =3D -EOPNOTSUPP; > > The error should probably be -EINVAL. > > It's possible that we may add an attribute that SELinux doesn't > support, say LSM_ATTR_CRYPTO_KEY, that another LSM does. This is > the same behavior the other LSMs exhibit in the face of a request > for attributes such as LSM_ATTR_KEYCREATE that they don't support. Okay, I'll accept that argument, but I would ask that add some additional handling in selinux_getprocattr() so that it returns -EINVAL in this case just as it does today. > >> goto bad; > >> } > >> rcu_read_unlock(); > >> @@ -6398,7 +6404,7 @@ static int selinux_getprocattr(struct task_struc= t *p, > >> return error; > >> } > >> > >> -static int selinux_setprocattr(const char *name, void *value, size_t = size) > >> +static int do_setattr(u64 attr, void *value, size_t size) > >> { > >> struct task_security_struct *tsec; > >> struct cred *new; > >> @@ -6409,28 +6415,36 @@ static int selinux_setprocattr(const char *nam= e, void *value, size_t size) > >> /* > >> * Basic control over ability to set these attributes at all. > >> */ > >> - if (!strcmp(name, "exec")) > >> + switch (attr) { > >> + case LSM_ATTR_CURRENT: > >> + error =3D avc_has_perm(&selinux_state, > >> + mysid, mysid, SECCLASS_PROCESS, > >> + PROCESS__SETCURRENT, NULL); > >> + break; > >> + case LSM_ATTR_EXEC: > >> error =3D avc_has_perm(&selinux_state, > >> mysid, mysid, SECCLASS_PROCESS, > >> PROCESS__SETEXEC, NULL); > >> - else if (!strcmp(name, "fscreate")) > >> + break; > >> + case LSM_ATTR_FSCREATE: > >> error =3D avc_has_perm(&selinux_state, > >> mysid, mysid, SECCLASS_PROCESS, > >> PROCESS__SETFSCREATE, NULL); > >> - else if (!strcmp(name, "keycreate")) > >> + break; > >> + case LSM_ATTR_KEYCREATE: > >> error =3D avc_has_perm(&selinux_state, > >> mysid, mysid, SECCLASS_PROCESS, > >> PROCESS__SETKEYCREATE, NULL); > >> - else if (!strcmp(name, "sockcreate")) > >> + break; > >> + case LSM_ATTR_SOCKCREATE: > >> error =3D avc_has_perm(&selinux_state, > >> mysid, mysid, SECCLASS_PROCESS, > >> PROCESS__SETSOCKCREATE, NULL); > >> - else if (!strcmp(name, "current")) > >> - error =3D avc_has_perm(&selinux_state, > >> - mysid, mysid, SECCLASS_PROCESS, > >> - PROCESS__SETCURRENT, NULL); > >> - else > >> - error =3D -EINVAL; > >> + break; > >> + default: > >> + error =3D -EOPNOTSUPP; > > Same as above, should be -EINVAL. > > Same as above, there may be attributes SELinux doesn't support. Also, same as above. > >> + break; > >> + } > >> if (error) > >> return error; > >> > >> @@ -6442,13 +6456,14 @@ static int selinux_setprocattr(const char *nam= e, void *value, size_t size) > >> } > >> error =3D security_context_to_sid(&selinux_state, valu= e, size, > >> &sid, GFP_KERNEL); > >> - if (error =3D=3D -EINVAL && !strcmp(name, "fscreate"))= { > >> + if (error =3D=3D -EINVAL && attr =3D=3D LSM_ATTR_FSCRE= ATE) { > >> if (!has_cap_mac_admin(true)) { > >> struct audit_buffer *ab; > >> size_t audit_size; > >> > >> - /* We strip a nul only if it is at the= end, otherwise the > >> - * context contains a nul and we shoul= d audit that */ > >> + /* We strip a nul only if it is at the= end, > >> + * otherwise the context contains a nu= l and > >> + * we should audit that */ > > You *do* get gold stars for fixing line lengths in close proximity ;) > > I guess I'm the Last User of the 80 character terminal. I'm still a big fan and I'm sticking to the 80 char limit for the LSM layer as well as the SELinux, audit, and labeled networking subsystems. Longer lines either predate me or I simply didn't catch them during review/merge. --=20 paul-moore.com