Message ID | 20230315224704.2672-8-casey@schaufler-ca.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp165875wrt; Wed, 15 Mar 2023 15:54:36 -0700 (PDT) X-Google-Smtp-Source: AK7set9JzlmILPRs6pJ1lYL/DQtKys116ykgS7E2Nxu1tTCx102mg7Ln/IoevAZZKy0R3TLx1oJR X-Received: by 2002:a17:903:1ca:b0:19e:bfec:7928 with SMTP id e10-20020a17090301ca00b0019ebfec7928mr1480856plh.24.1678920876410; Wed, 15 Mar 2023 15:54:36 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1678920876; cv=none; d=google.com; s=arc-20160816; b=ISWChm4NkWwfVeumfdgarv27/SPiCjwfNbK6bgOL7QuDoDPDbAnU93ucqgDMdvOsTF S0rObPKSJTFq0db5vuDivMyG3nP9bF4+V8k1tCqYiQLTQ/NV/kwHw191KjMHVh7iU1GD MLjUFRay2fhTl5e5BoSIaOQJPpSY5nVw3bKCX+7hCAOio9IkleeUGDrFHlW7lqo5GLmU E+FadLQqYYpG3ckamjbxShu0d0Cm2JOl+bq8xWjLxsNh7d7IxYiIjjycV1b9vR9sYiCb omDEWrYb/2JzTAy+/968tMqqpDUi3+OElfLXzG5mdOTmqwOEVmKxCe/vQSOrl/5ewhBl Htug== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=iOXNKeJzMcKJmHYMnw6KavWs/Fn7DObPOeEurKOnwpE=; b=iau0+bXGRUJOaRED4EjkSsuMNERYKUsde7TIoM/eoExEODFcX5q7y/B8NaWWBislNy VpI72jqozdtuKij+CoBno3JeTuhOaX8pUbOlfY0DADtckWrYZh6VurKUJjJC2ymK8blR YDnC6ShqxMZsBJz47JNa8XXUuvjEQDPOTJSqU3pFDVsJ8/T3NLbgOgaBmZTk4WtiqSOr VpRAolAMIr8bi6XfkeYVP4r4Hkv45esbKbtYNgpk1oJTv5PxLdscN2Eh0z7lp4y53839 5w+HaVoBi1+NMy10/5hUpp5eEdBf7oMZdF3lM0iYVfs7jCjyGNVjSYrCuPXTsMbxiM1B jxjg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@yahoo.com header.s=s2048 header.b=Iurpvham; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id jg20-20020a17090326d400b0019ea0e227f0si6015243plb.296.2023.03.15.15.54.21; Wed, 15 Mar 2023 15:54:36 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@yahoo.com header.s=s2048 header.b=Iurpvham; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232351AbjCOWvd (ORCPT <rfc822;ruipengqi7@gmail.com> + 99 others); Wed, 15 Mar 2023 18:51:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47674 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230404AbjCOWv1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 15 Mar 2023 18:51:27 -0400 Received: from sonic309-27.consmr.mail.ne1.yahoo.com (sonic309-27.consmr.mail.ne1.yahoo.com [66.163.184.153]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1421A231E6 for <linux-kernel@vger.kernel.org>; Wed, 15 Mar 2023 15:50:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1678920627; bh=iOXNKeJzMcKJmHYMnw6KavWs/Fn7DObPOeEurKOnwpE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From:Subject:Reply-To; b=Iurpvham6ycqSSOXCkko2LlwD8dTg62HpCQ3oasx8UF3hW6g+0DDnrrHBKZeMb2A4ojCIrmAyjqEzBDsXf3kfgsMZ7f1I4bQ5H4oP3JG4l2kyOlrfrPiXHvs2tZdYci0MBWVFz30vWIim2t5wFQTebNOad6f3lQXrGzNK/Wrgxl0dlUHfLBO0P99AHIvqEkiKpNr6PW89ULbrLHDkvgVYM+Z/EY4W4NPadC5YX02HKDrAS+rRlblziGzlQztGX3vc4B554XMiYSEfPdgkEGJlUsEnaO31qLwOYL6VX0MUAmcnS3HhyBqvXuIkUMUynKAfB6S3vP19brSfQRu6bdEGw== X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1678920627; bh=MeNtSSqaIbKawRDtgUERAoJZpzOiiuSjYSBt0+7uQ80=; h=X-Sonic-MF:From:To:Subject:Date:From:Subject; b=DFfcOcU+sK9P88R0EhcpSZdH/nbtjuOOni4C/n6GMs2FknHhZY1BGIuiWNJZmPMrze9HJdB0zTnkuF1XsG+fq3qHZ8oDfjDfslgquwoK0yqrxEcGCd1ol3Jd132gKWe0joFsmLaWd+oReNaZ5hyZG6SkpGOQs/DO/GSd/DKeF9N4mycyg4EnCFjSa187ttN88V7070wPLVJiziezr1khd+7D7En6IiWCLteIUd+6r+c4gaIa0HDFavIuwfZaEA4S1wi5mkNesxhFAg9AL00GGk/PffIEQYSXNwIIwfBrqve0r421mbiLteF6C64ciBHHz7fGHl0mfh6gcb1slI6NVw== X-YMail-OSG: VNuHjVUVM1nWpZv.HX_1Lrx93NAoyPbqGRVK9unYFMDj4InAJjauhsZuXfc0w6a PPCXlNImomlZ0n9sS2Dz2JAPjjB4Nl7Zo3YtYbtdlyUDVAL8rssBiLTidEby9GvJInbAf8PrIPuh 6VPoYFjbnyOtYnVevLZ86x_1xxrnE0VBwND6r4M2GKa2vFeW02s3bqZqPTWTupK4yrSWF.PWrMme epgHiQowchn2I1IDJ_WEvIUgP9TY.hRtjTSPd1gaHK4k1tAvmLThuwu.SCCLil_S42rnulbzEkaE Se2rtvOd5u3QjKRy1yWxj5eav6kCJPaWUNnwTmv47BaWadfqUlfEsLw_H2OSLLBD4x9yXAdYtEw4 GayddkmqEpv3r5iu4JNoCs23MJc5Mz1mqqtX4jUfeSvQLFtq1H.Hc8KUu4Ff4YTTp0zdYzfPvBQV QbveOkf6zhYSfSf_B99RkjB0Wu.OJQofZZn3XGV5xI8NfTjxZQ_mok5n8Kgyi4U7f1OFStz0kOhu LOAeBXbIjuZr0uXm3bSWCAtad338MLBeW4eP7opmN2Et6Q5FlgyMxR8pnHs.E9_zhC5Kk24F9Vgs hegjyPAHENubPDcIHwsj4TYQo242BXMmnMoMiqIHEsIHsLnfLXA6eQ7pqgUSCmx1BPFvDvMDI20d IqKd8zcTiOjksmQ_zK1TnN20JfEHysZ_HcLt.vmaXi3bs0W7OEQ1uciH6iCDvwGtQOnfGmNg7gFA CpBX5r9UynE88FCikneT1iEmNEud7YdH.WutiqFqolGdCl0KiQQ0BGoQBFb7WWgDC4_7Kt6x8_6E 2pnHUDF9hSl7VhJdWUtH77JXqw5kBCY90QfFwwb2Pp1EmSyg2R3mTC0doA8rsWg_HXugILZCOTqm 2BXvoh4rjI8N1gbzGPSAagpxAwym8ARtNesYfXj8DyoPjhTnOF9QN3_znLpf11UPNQGvb9d4A2E_ W.NjtG1q1ZqRR0hPp_nhzq_2dYP8Y_hJ2_poW1CIYVDRrPITykjYIFsIX10GEOGzyvO4_SdoSgPS xu6taApz0I3fyWyO67Bewc.mwJs6.XgfL54eWjq1ZhSjKV8XjH7evZA2XWXJHc.DVbbDJvJcSEQA H3moJMq4BVNoFj1rAo8f2BfGfxSTvKjQcCwyof3AdX4EdS_j9J.YuFCo9MOT1avB.AkFBB61OuIr 9dFUNFnuxj8mdNyRZsfVpUr3qoKxrOJGAVJ1HtARz0_mz2YeEkQrTscyIAk95znxA29hp52SONp. o8M3MaYZwcRblQlxdAcLE.umltQ7gE.htpCwC7HeZM2Wl4MPNDi3qqGq9.uVe6vuJ5.M7ZzRWSPb 7JOfbZQJ8FYQzDc2E0RR1bGKHDqdV0V3WWfE1UdFNC7RI0hEnb6US2yMpOiLIAz7arAyXxf65b1j 7F8o0KFcezShaYFBoCPMIpKtPrBivvF4MKbENcnHnFi5RnBwS2HXBbzmYaOvya.ZMt6Pa7PGN.M5 jy.RTKX6Rc.APuTAH8DGWeCwvXinsLpfbUGG1rn7zGI8HgR1OCl3HLJdLDuuChICtGUMOjMZBzDf kVfZSzW8x0naso8bMgnzXFILFgn.nuxjp2Y5y6qqDC_eyCESbk4vA7AAdh5piKT8SpgkrywcZJLF ClJz8Fh1.JH6jo7Q_LSvymTCY7u_HrnBpyc6IbApZW7Uk.X.rxtMsf5cbb5hbP_iYprwZKjMs67g y3nEDjPQ7bIlbodaMT2CokGWAZDv9SKiRhKwUDZ53e1k7GYzgLcmr04RpYZdVv1IG.2nPgNqyCB7 5nOypnT4Rje2.OebxKK4JWWPpi5NcBRmyT76PNpEOKFrW2uRVfvhd0Mg53IVNwkocxj__6F_mKg6 PitfDyX1fKXmE80WOTRnv.utrXX.I03xejpmgsUW0pVCMLNuT53qiJuNMK7PQMT5X7fwOLbJB8Wd FZl5ClxER7ZXp2MStsuQIe1zzvzww4jJi.yfYHK0xdEh67JcTu9y_mjvmhC7r2pyMSpFRaUSrz77 qZLdTGc1i67elQLswWlF897N0nHCsPUjLG2BQGYH5J9TAAE6GWeWXt7HmEDapzteqncl4KGk1bWU DO3qvdT3N.mpW8bFCN.Pn2DM4MhH2VyvFEx1l9mxSAoaM4rcG0ILR28HmRECB.2xVx6Z9k2xr5WT nO2f7awihbZsqQqa_84ubHY5RJpVVOT6MeMOu7VtpLEnFRXS_9DEf2hYGf1TTlMSIPC7mVz5_fS0 hFhWJNTDmdEdgeVJZqRJwYEM6.wDsJ.BSGqgJCTTa X-Sonic-MF: <casey@schaufler-ca.com> X-Sonic-ID: bd2ea56c-66c7-4243-89d2-b3d3e39a8c79 Received: from sonic.gate.mail.ne1.yahoo.com by sonic309.consmr.mail.ne1.yahoo.com with HTTP; Wed, 15 Mar 2023 22:50:27 +0000 Received: by hermes--production-bf1-777648578f-mpdrm (Yahoo Inc. Hermes SMTP Server) with ESMTPA ID 50e2f089a68eaebcc40c76391bc13d32; Wed, 15 Mar 2023 22:50:22 +0000 (UTC) From: Casey Schaufler <casey@schaufler-ca.com> To: casey@schaufler-ca.com, paul@paul-moore.com, linux-security-module@vger.kernel.org Cc: 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 Subject: [PATCH v7 07/11] LSM: Helpers for attribute names and filling an lsm_ctx Date: Wed, 15 Mar 2023 15:47:00 -0700 Message-Id: <20230315224704.2672-8-casey@schaufler-ca.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20230315224704.2672-1-casey@schaufler-ca.com> References: <20230315224704.2672-1-casey@schaufler-ca.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760476136737967680?= X-GMAIL-MSGID: =?utf-8?q?1760476136737967680?= |
Series |
LSM: Three basic syscalls
|
|
Commit Message
Casey Schaufler
March 15, 2023, 10:47 p.m. UTC
Add lsm_name_to_attr(), which translates a text string to a
LSM_ATTR value if one is available.
Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
the trailing attribute value.
All are used in module specific components of LSM system calls.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
include/linux/security.h | 13 ++++++++++
security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
security/security.c | 31 ++++++++++++++++++++++++
3 files changed, 95 insertions(+)
Comments
On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Add lsm_name_to_attr(), which translates a text string to a > LSM_ATTR value if one is available. > > Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > the trailing attribute value. > > All are used in module specific components of LSM system calls. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/security.h | 13 ++++++++++ > security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > security/security.c | 31 ++++++++++++++++++++++++ > 3 files changed, 95 insertions(+) ... > diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > index 6efbe244d304..55d849ad5d6e 100644 > --- a/security/lsm_syscalls.c > +++ b/security/lsm_syscalls.c > @@ -17,6 +17,57 @@ > #include <linux/lsm_hooks.h> > #include <uapi/linux/lsm.h> > > +struct attr_map { > + char *name; > + u64 attr; > +}; > + > +static const struct attr_map lsm_attr_names[] = { > + { > + .name = "current", > + .attr = LSM_ATTR_CURRENT, > + }, > + { > + .name = "exec", > + .attr = LSM_ATTR_EXEC, > + }, > + { > + .name = "fscreate", > + .attr = LSM_ATTR_FSCREATE, > + }, > + { > + .name = "keycreate", > + .attr = LSM_ATTR_KEYCREATE, > + }, > + { > + .name = "prev", > + .attr = LSM_ATTR_PREV, > + }, > + { > + .name = "sockcreate", > + .attr = LSM_ATTR_SOCKCREATE, > + }, > +}; > + > +/** > + * lsm_name_to_attr - map an LSM attribute name to its ID > + * @name: name of the attribute > + * > + * Look the given @name up in the table of know attribute names. > + * > + * Returns the LSM attribute value associated with @name, or 0 if > + * there is no mapping. > + */ > +u64 lsm_name_to_attr(const char *name) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > + if (!strcmp(name, lsm_attr_names[i].name)) > + return lsm_attr_names[i].attr; I'm pretty sure this is the only place where @lsm_attr_names is used, right? If true, when coupled with the idea that these syscalls are going to close the door on new LSM attributes in procfs I think we can just put the mapping directly in this function via a series of if-statements. > + return 0; > +} > + > /** > * sys_lsm_set_self_attr - Set current task's security module attribute > * @attr: which attribute to set > diff --git a/security/security.c b/security/security.c > index 2c57fe28c4f7..f7b814a3940c 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) > return 0; > } > > +/** > + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > + * @ctx: an LSM context to be filled > + * @context: the new context value > + * @context_size: the size of the new context value > + * @id: LSM id > + * @flags: LSM defined flags > + * > + * Fill all of the fields in a user space lsm_ctx structure. > + * Caller is assumed to have verified that @ctx has enough space > + * for @context. > + * Returns 0 on success, -EFAULT on a copyout error. > + */ > +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > + size_t context_size, u64 id, u64 flags) > +{ > + struct lsm_ctx local; > + void __user *vc = ctx; > + > + local.id = id; > + local.flags = flags; > + local.ctx_len = context_size; > + local.len = context_size + sizeof(local); > + vc += sizeof(local); See my prior comments about void pointer math. > + if (copy_to_user(ctx, &local, sizeof(local))) > + return -EFAULT; > + if (context_size > 0 && copy_to_user(vc, context, context_size)) > + return -EFAULT; Should we handle the padding in this function? > + return 0; > +} -- paul-moore.com
On 3/29/2023 6:13 PM, Paul Moore wrote: > On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> Add lsm_name_to_attr(), which translates a text string to a >> LSM_ATTR value if one is available. >> >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >> the trailing attribute value. >> >> All are used in module specific components of LSM system calls. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 13 ++++++++++ >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >> security/security.c | 31 ++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+) > .. > >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c >> index 6efbe244d304..55d849ad5d6e 100644 >> --- a/security/lsm_syscalls.c >> +++ b/security/lsm_syscalls.c >> @@ -17,6 +17,57 @@ >> #include <linux/lsm_hooks.h> >> #include <uapi/linux/lsm.h> >> >> +struct attr_map { >> + char *name; >> + u64 attr; >> +}; >> + >> +static const struct attr_map lsm_attr_names[] = { >> + { >> + .name = "current", >> + .attr = LSM_ATTR_CURRENT, >> + }, >> + { >> + .name = "exec", >> + .attr = LSM_ATTR_EXEC, >> + }, >> + { >> + .name = "fscreate", >> + .attr = LSM_ATTR_FSCREATE, >> + }, >> + { >> + .name = "keycreate", >> + .attr = LSM_ATTR_KEYCREATE, >> + }, >> + { >> + .name = "prev", >> + .attr = LSM_ATTR_PREV, >> + }, >> + { >> + .name = "sockcreate", >> + .attr = LSM_ATTR_SOCKCREATE, >> + }, >> +}; >> + >> +/** >> + * lsm_name_to_attr - map an LSM attribute name to its ID >> + * @name: name of the attribute >> + * >> + * Look the given @name up in the table of know attribute names. >> + * >> + * Returns the LSM attribute value associated with @name, or 0 if >> + * there is no mapping. >> + */ >> +u64 lsm_name_to_attr(const char *name) >> +{ >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) >> + if (!strcmp(name, lsm_attr_names[i].name)) >> + return lsm_attr_names[i].attr; > I'm pretty sure this is the only place where @lsm_attr_names is used, > right? If true, when coupled with the idea that these syscalls are > going to close the door on new LSM attributes in procfs I think we can > just put the mapping directly in this function via a series of > if-statements. Ick. You're not wrong, but the hard coded if-statement approach goes against all sorts of coding principles. I'll do it, but I can't say I like it. >> + return 0; >> +} >> + >> /** >> * sys_lsm_set_self_attr - Set current task's security module attribute >> * @attr: which attribute to set >> diff --git a/security/security.c b/security/security.c >> index 2c57fe28c4f7..f7b814a3940c 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) >> return 0; >> } >> >> +/** >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >> + * @ctx: an LSM context to be filled >> + * @context: the new context value >> + * @context_size: the size of the new context value >> + * @id: LSM id >> + * @flags: LSM defined flags >> + * >> + * Fill all of the fields in a user space lsm_ctx structure. >> + * Caller is assumed to have verified that @ctx has enough space >> + * for @context. >> + * Returns 0 on success, -EFAULT on a copyout error. >> + */ >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >> + size_t context_size, u64 id, u64 flags) >> +{ >> + struct lsm_ctx local; >> + void __user *vc = ctx; >> + >> + local.id = id; >> + local.flags = flags; >> + local.ctx_len = context_size; >> + local.len = context_size + sizeof(local); >> + vc += sizeof(local); > See my prior comments about void pointer math. > >> + if (copy_to_user(ctx, &local, sizeof(local))) >> + return -EFAULT; >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >> + return -EFAULT; > Should we handle the padding in this function? This function fills in a lsm_ctx. The padding, if any, is in addition to the lsm_ctx, not part of it. >> + return 0; >> +} > -- > paul-moore.com
On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 3/29/2023 6:13 PM, Paul Moore wrote: > > On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> Add lsm_name_to_attr(), which translates a text string to a > >> LSM_ATTR value if one is available. > >> > >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > >> the trailing attribute value. > >> > >> All are used in module specific components of LSM system calls. > >> > >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >> --- > >> include/linux/security.h | 13 ++++++++++ > >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > >> security/security.c | 31 ++++++++++++++++++++++++ > >> 3 files changed, 95 insertions(+) > > .. > > > >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > >> index 6efbe244d304..55d849ad5d6e 100644 > >> --- a/security/lsm_syscalls.c > >> +++ b/security/lsm_syscalls.c > >> @@ -17,6 +17,57 @@ > >> #include <linux/lsm_hooks.h> > >> #include <uapi/linux/lsm.h> > >> > >> +struct attr_map { > >> + char *name; > >> + u64 attr; > >> +}; > >> + > >> +static const struct attr_map lsm_attr_names[] = { > >> + { > >> + .name = "current", > >> + .attr = LSM_ATTR_CURRENT, > >> + }, > >> + { > >> + .name = "exec", > >> + .attr = LSM_ATTR_EXEC, > >> + }, > >> + { > >> + .name = "fscreate", > >> + .attr = LSM_ATTR_FSCREATE, > >> + }, > >> + { > >> + .name = "keycreate", > >> + .attr = LSM_ATTR_KEYCREATE, > >> + }, > >> + { > >> + .name = "prev", > >> + .attr = LSM_ATTR_PREV, > >> + }, > >> + { > >> + .name = "sockcreate", > >> + .attr = LSM_ATTR_SOCKCREATE, > >> + }, > >> +}; > >> + > >> +/** > >> + * lsm_name_to_attr - map an LSM attribute name to its ID > >> + * @name: name of the attribute > >> + * > >> + * Look the given @name up in the table of know attribute names. > >> + * > >> + * Returns the LSM attribute value associated with @name, or 0 if > >> + * there is no mapping. > >> + */ > >> +u64 lsm_name_to_attr(const char *name) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > >> + if (!strcmp(name, lsm_attr_names[i].name)) > >> + return lsm_attr_names[i].attr; > > I'm pretty sure this is the only place where @lsm_attr_names is used, > > right? If true, when coupled with the idea that these syscalls are > > going to close the door on new LSM attributes in procfs I think we can > > just put the mapping directly in this function via a series of > > if-statements. > > Ick. You're not wrong, but the hard coded if-statement approach goes > against all sorts of coding principles. I'll do it, but I can't say I > like it. If it helps any, I understand and am sympathetic. I guess I've gotten to that point where in addition to "code elegance", I'm also very concerned about defending against "code abuse", and something like an nicely defined mapping array is ripe for someone to come along and use that to justify further use of the attribute string names in some other function/API. If you want to stick with the array - I have no problem with that - make it local to lsm_name_to_attr(). > >> +/** > >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > >> + * @ctx: an LSM context to be filled > >> + * @context: the new context value > >> + * @context_size: the size of the new context value > >> + * @id: LSM id > >> + * @flags: LSM defined flags > >> + * > >> + * Fill all of the fields in a user space lsm_ctx structure. > >> + * Caller is assumed to have verified that @ctx has enough space > >> + * for @context. > >> + * Returns 0 on success, -EFAULT on a copyout error. > >> + */ > >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > >> + size_t context_size, u64 id, u64 flags) > >> +{ > >> + struct lsm_ctx local; > >> + void __user *vc = ctx; > >> + > >> + local.id = id; > >> + local.flags = flags; > >> + local.ctx_len = context_size; > >> + local.len = context_size + sizeof(local); > >> + vc += sizeof(local); > > See my prior comments about void pointer math. > > > >> + if (copy_to_user(ctx, &local, sizeof(local))) > >> + return -EFAULT; > >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) > >> + return -EFAULT; > > Should we handle the padding in this function? > > This function fills in a lsm_ctx. The padding, if any, is in addition to > the lsm_ctx, not part of it. Okay, so where is the padding managed? I may have missed it, but I don't recall seeing it anywhere in this patchset ...
On 3/30/2023 4:28 PM, Paul Moore wrote: > On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 3/29/2023 6:13 PM, Paul Moore wrote: >>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> Add lsm_name_to_attr(), which translates a text string to a >>>> LSM_ATTR value if one is available. >>>> >>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>>> the trailing attribute value. >>>> >>>> All are used in module specific components of LSM system calls. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> --- >>>> include/linux/security.h | 13 ++++++++++ >>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>>> security/security.c | 31 ++++++++++++++++++++++++ >>>> 3 files changed, 95 insertions(+) >>> .. >>> >>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c >>>> index 6efbe244d304..55d849ad5d6e 100644 >>>> --- a/security/lsm_syscalls.c >>>> +++ b/security/lsm_syscalls.c >>>> @@ -17,6 +17,57 @@ >>>> #include <linux/lsm_hooks.h> >>>> #include <uapi/linux/lsm.h> >>>> >>>> +struct attr_map { >>>> + char *name; >>>> + u64 attr; >>>> +}; >>>> + >>>> +static const struct attr_map lsm_attr_names[] = { >>>> + { >>>> + .name = "current", >>>> + .attr = LSM_ATTR_CURRENT, >>>> + }, >>>> + { >>>> + .name = "exec", >>>> + .attr = LSM_ATTR_EXEC, >>>> + }, >>>> + { >>>> + .name = "fscreate", >>>> + .attr = LSM_ATTR_FSCREATE, >>>> + }, >>>> + { >>>> + .name = "keycreate", >>>> + .attr = LSM_ATTR_KEYCREATE, >>>> + }, >>>> + { >>>> + .name = "prev", >>>> + .attr = LSM_ATTR_PREV, >>>> + }, >>>> + { >>>> + .name = "sockcreate", >>>> + .attr = LSM_ATTR_SOCKCREATE, >>>> + }, >>>> +}; >>>> + >>>> +/** >>>> + * lsm_name_to_attr - map an LSM attribute name to its ID >>>> + * @name: name of the attribute >>>> + * >>>> + * Look the given @name up in the table of know attribute names. >>>> + * >>>> + * Returns the LSM attribute value associated with @name, or 0 if >>>> + * there is no mapping. >>>> + */ >>>> +u64 lsm_name_to_attr(const char *name) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) >>>> + if (!strcmp(name, lsm_attr_names[i].name)) >>>> + return lsm_attr_names[i].attr; >>> I'm pretty sure this is the only place where @lsm_attr_names is used, >>> right? If true, when coupled with the idea that these syscalls are >>> going to close the door on new LSM attributes in procfs I think we can >>> just put the mapping directly in this function via a series of >>> if-statements. >> Ick. You're not wrong, but the hard coded if-statement approach goes >> against all sorts of coding principles. I'll do it, but I can't say I >> like it. > If it helps any, I understand and am sympathetic. I guess I've gotten > to that point where in addition to "code elegance", I'm also very > concerned about defending against "code abuse", and something like an > nicely defined mapping array is ripe for someone to come along and use > that to justify further use of the attribute string names in some > other function/API. > > If you want to stick with the array - I have no problem with that - > make it local to lsm_name_to_attr(). > >>>> +/** >>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>>> + * @ctx: an LSM context to be filled >>>> + * @context: the new context value >>>> + * @context_size: the size of the new context value >>>> + * @id: LSM id >>>> + * @flags: LSM defined flags >>>> + * >>>> + * Fill all of the fields in a user space lsm_ctx structure. >>>> + * Caller is assumed to have verified that @ctx has enough space >>>> + * for @context. >>>> + * Returns 0 on success, -EFAULT on a copyout error. >>>> + */ >>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>>> + size_t context_size, u64 id, u64 flags) >>>> +{ >>>> + struct lsm_ctx local; >>>> + void __user *vc = ctx; >>>> + >>>> + local.id = id; >>>> + local.flags = flags; >>>> + local.ctx_len = context_size; >>>> + local.len = context_size + sizeof(local); >>>> + vc += sizeof(local); >>> See my prior comments about void pointer math. >>> >>>> + if (copy_to_user(ctx, &local, sizeof(local))) >>>> + return -EFAULT; >>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>>> + return -EFAULT; >>> Should we handle the padding in this function? >> This function fills in a lsm_ctx. The padding, if any, is in addition to >> the lsm_ctx, not part of it. > Okay, so where is the padding managed? I may have missed it, but I > don't recall seeing it anywhere in this patchset ... Padding isn't being managed. There has been talk about using padding to expand the API, but there is no use for it now. Or is there?
On Fri, Mar 31, 2023 at 12:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 3/30/2023 4:28 PM, Paul Moore wrote: > > On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> On 3/29/2023 6:13 PM, Paul Moore wrote: > >>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >>>> Add lsm_name_to_attr(), which translates a text string to a > >>>> LSM_ATTR value if one is available. > >>>> > >>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > >>>> the trailing attribute value. > >>>> > >>>> All are used in module specific components of LSM system calls. > >>>> > >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > >>>> --- > >>>> include/linux/security.h | 13 ++++++++++ > >>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > >>>> security/security.c | 31 ++++++++++++++++++++++++ > >>>> 3 files changed, 95 insertions(+) > >>> .. > >>> > >>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > >>>> index 6efbe244d304..55d849ad5d6e 100644 > >>>> --- a/security/lsm_syscalls.c > >>>> +++ b/security/lsm_syscalls.c > >>>> @@ -17,6 +17,57 @@ > >>>> #include <linux/lsm_hooks.h> > >>>> #include <uapi/linux/lsm.h> > >>>> > >>>> +struct attr_map { > >>>> + char *name; > >>>> + u64 attr; > >>>> +}; > >>>> + > >>>> +static const struct attr_map lsm_attr_names[] = { > >>>> + { > >>>> + .name = "current", > >>>> + .attr = LSM_ATTR_CURRENT, > >>>> + }, > >>>> + { > >>>> + .name = "exec", > >>>> + .attr = LSM_ATTR_EXEC, > >>>> + }, > >>>> + { > >>>> + .name = "fscreate", > >>>> + .attr = LSM_ATTR_FSCREATE, > >>>> + }, > >>>> + { > >>>> + .name = "keycreate", > >>>> + .attr = LSM_ATTR_KEYCREATE, > >>>> + }, > >>>> + { > >>>> + .name = "prev", > >>>> + .attr = LSM_ATTR_PREV, > >>>> + }, > >>>> + { > >>>> + .name = "sockcreate", > >>>> + .attr = LSM_ATTR_SOCKCREATE, > >>>> + }, > >>>> +}; > >>>> + > >>>> +/** > >>>> + * lsm_name_to_attr - map an LSM attribute name to its ID > >>>> + * @name: name of the attribute > >>>> + * > >>>> + * Look the given @name up in the table of know attribute names. > >>>> + * > >>>> + * Returns the LSM attribute value associated with @name, or 0 if > >>>> + * there is no mapping. > >>>> + */ > >>>> +u64 lsm_name_to_attr(const char *name) > >>>> +{ > >>>> + int i; > >>>> + > >>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > >>>> + if (!strcmp(name, lsm_attr_names[i].name)) > >>>> + return lsm_attr_names[i].attr; > >>> I'm pretty sure this is the only place where @lsm_attr_names is used, > >>> right? If true, when coupled with the idea that these syscalls are > >>> going to close the door on new LSM attributes in procfs I think we can > >>> just put the mapping directly in this function via a series of > >>> if-statements. > >> Ick. You're not wrong, but the hard coded if-statement approach goes > >> against all sorts of coding principles. I'll do it, but I can't say I > >> like it. > > If it helps any, I understand and am sympathetic. I guess I've gotten > > to that point where in addition to "code elegance", I'm also very > > concerned about defending against "code abuse", and something like an > > nicely defined mapping array is ripe for someone to come along and use > > that to justify further use of the attribute string names in some > > other function/API. > > > > If you want to stick with the array - I have no problem with that - > > make it local to lsm_name_to_attr(). > > > >>>> +/** > >>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > >>>> + * @ctx: an LSM context to be filled > >>>> + * @context: the new context value > >>>> + * @context_size: the size of the new context value > >>>> + * @id: LSM id > >>>> + * @flags: LSM defined flags > >>>> + * > >>>> + * Fill all of the fields in a user space lsm_ctx structure. > >>>> + * Caller is assumed to have verified that @ctx has enough space > >>>> + * for @context. > >>>> + * Returns 0 on success, -EFAULT on a copyout error. > >>>> + */ > >>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > >>>> + size_t context_size, u64 id, u64 flags) > >>>> +{ > >>>> + struct lsm_ctx local; > >>>> + void __user *vc = ctx; > >>>> + > >>>> + local.id = id; > >>>> + local.flags = flags; > >>>> + local.ctx_len = context_size; > >>>> + local.len = context_size + sizeof(local); > >>>> + vc += sizeof(local); > >>> See my prior comments about void pointer math. > >>> > >>>> + if (copy_to_user(ctx, &local, sizeof(local))) > >>>> + return -EFAULT; > >>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) > >>>> + return -EFAULT; > >>> Should we handle the padding in this function? > >> This function fills in a lsm_ctx. The padding, if any, is in addition to > >> the lsm_ctx, not part of it. > > Okay, so where is the padding managed? I may have missed it, but I > > don't recall seeing it anywhere in this patchset ... > > Padding isn't being managed. There has been talk about using padding to > expand the API, but there is no use for it now. Or is there? I think two separate ideas are getting conflated, likely because the 'len' field is involved in both. THe first issue is padding at the end of the lsm_ctx struct to ensure that the next array element is aligned. The second issue is the potential for extending the lsm_ctx struct on a per-LSM basis through creative use of the 'flags' and 'len' fields; in this case additional information could be stashed at the end of the lsm_ctx struct after the 'ctx' field. The latter issue (extending the lsm_ctx) isn't something we want to jump into, but it is something the syscall/struct API would allow, and I don't want to exclude it as a possible future solution to a yet unknown future problem. The former issue (padding array elements) isn't a strict requirement as the syscall/struct API works either way, but it seems like a good thing to do.
On 3/31/2023 12:24 PM, Paul Moore wrote: > On Fri, Mar 31, 2023 at 12:56 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 3/30/2023 4:28 PM, Paul Moore wrote: >>> On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> On 3/29/2023 6:13 PM, Paul Moore wrote: >>>>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>>> Add lsm_name_to_attr(), which translates a text string to a >>>>>> LSM_ATTR value if one is available. >>>>>> >>>>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>>>>> the trailing attribute value. >>>>>> >>>>>> All are used in module specific components of LSM system calls. >>>>>> >>>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>>>> --- >>>>>> include/linux/security.h | 13 ++++++++++ >>>>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>>>>> security/security.c | 31 ++++++++++++++++++++++++ >>>>>> 3 files changed, 95 insertions(+) >>>>> .. >>>>> >>>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c >>>>>> index 6efbe244d304..55d849ad5d6e 100644 >>>>>> --- a/security/lsm_syscalls.c >>>>>> +++ b/security/lsm_syscalls.c >>>>>> @@ -17,6 +17,57 @@ >>>>>> #include <linux/lsm_hooks.h> >>>>>> #include <uapi/linux/lsm.h> >>>>>> >>>>>> +struct attr_map { >>>>>> + char *name; >>>>>> + u64 attr; >>>>>> +}; >>>>>> + >>>>>> +static const struct attr_map lsm_attr_names[] = { >>>>>> + { >>>>>> + .name = "current", >>>>>> + .attr = LSM_ATTR_CURRENT, >>>>>> + }, >>>>>> + { >>>>>> + .name = "exec", >>>>>> + .attr = LSM_ATTR_EXEC, >>>>>> + }, >>>>>> + { >>>>>> + .name = "fscreate", >>>>>> + .attr = LSM_ATTR_FSCREATE, >>>>>> + }, >>>>>> + { >>>>>> + .name = "keycreate", >>>>>> + .attr = LSM_ATTR_KEYCREATE, >>>>>> + }, >>>>>> + { >>>>>> + .name = "prev", >>>>>> + .attr = LSM_ATTR_PREV, >>>>>> + }, >>>>>> + { >>>>>> + .name = "sockcreate", >>>>>> + .attr = LSM_ATTR_SOCKCREATE, >>>>>> + }, >>>>>> +}; >>>>>> + >>>>>> +/** >>>>>> + * lsm_name_to_attr - map an LSM attribute name to its ID >>>>>> + * @name: name of the attribute >>>>>> + * >>>>>> + * Look the given @name up in the table of know attribute names. >>>>>> + * >>>>>> + * Returns the LSM attribute value associated with @name, or 0 if >>>>>> + * there is no mapping. >>>>>> + */ >>>>>> +u64 lsm_name_to_attr(const char *name) >>>>>> +{ >>>>>> + int i; >>>>>> + >>>>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) >>>>>> + if (!strcmp(name, lsm_attr_names[i].name)) >>>>>> + return lsm_attr_names[i].attr; >>>>> I'm pretty sure this is the only place where @lsm_attr_names is used, >>>>> right? If true, when coupled with the idea that these syscalls are >>>>> going to close the door on new LSM attributes in procfs I think we can >>>>> just put the mapping directly in this function via a series of >>>>> if-statements. >>>> Ick. You're not wrong, but the hard coded if-statement approach goes >>>> against all sorts of coding principles. I'll do it, but I can't say I >>>> like it. >>> If it helps any, I understand and am sympathetic. I guess I've gotten >>> to that point where in addition to "code elegance", I'm also very >>> concerned about defending against "code abuse", and something like an >>> nicely defined mapping array is ripe for someone to come along and use >>> that to justify further use of the attribute string names in some >>> other function/API. >>> >>> If you want to stick with the array - I have no problem with that - >>> make it local to lsm_name_to_attr(). >>> >>>>>> +/** >>>>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>>>>> + * @ctx: an LSM context to be filled >>>>>> + * @context: the new context value >>>>>> + * @context_size: the size of the new context value >>>>>> + * @id: LSM id >>>>>> + * @flags: LSM defined flags >>>>>> + * >>>>>> + * Fill all of the fields in a user space lsm_ctx structure. >>>>>> + * Caller is assumed to have verified that @ctx has enough space >>>>>> + * for @context. >>>>>> + * Returns 0 on success, -EFAULT on a copyout error. >>>>>> + */ >>>>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>>>>> + size_t context_size, u64 id, u64 flags) >>>>>> +{ >>>>>> + struct lsm_ctx local; >>>>>> + void __user *vc = ctx; >>>>>> + >>>>>> + local.id = id; >>>>>> + local.flags = flags; >>>>>> + local.ctx_len = context_size; >>>>>> + local.len = context_size + sizeof(local); >>>>>> + vc += sizeof(local); >>>>> See my prior comments about void pointer math. >>>>> >>>>>> + if (copy_to_user(ctx, &local, sizeof(local))) >>>>>> + return -EFAULT; >>>>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>>>>> + return -EFAULT; >>>>> Should we handle the padding in this function? >>>> This function fills in a lsm_ctx. The padding, if any, is in addition to >>>> the lsm_ctx, not part of it. >>> Okay, so where is the padding managed? I may have missed it, but I >>> don't recall seeing it anywhere in this patchset ... >> Padding isn't being managed. There has been talk about using padding to >> expand the API, but there is no use for it now. Or is there? > I think two separate ideas are getting conflated, likely because the > 'len' field is involved in both. > > THe first issue is padding at the end of the lsm_ctx struct to ensure > that the next array element is aligned. The second issue is the > potential for extending the lsm_ctx struct on a per-LSM basis through > creative use of the 'flags' and 'len' fields; in this case additional > information could be stashed at the end of the lsm_ctx struct after > the 'ctx' field. The latter issue (extending the lsm_ctx) isn't > something we want to jump into, but it is something the syscall/struct > API would allow, and I don't want to exclude it as a possible future > solution to a yet unknown future problem. The former issue (padding > array elements) isn't a strict requirement as the syscall/struct API > works either way, but it seems like a good thing to do. Reasonable. Thanks for the clarification.
On 15/03/2023 23:47, Casey Schaufler wrote: > Add lsm_name_to_attr(), which translates a text string to a > LSM_ATTR value if one is available. > > Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including > the trailing attribute value. > > All are used in module specific components of LSM system calls. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/security.h | 13 ++++++++++ > security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ > security/security.c | 31 ++++++++++++++++++++++++ > 3 files changed, 95 insertions(+) [...] > diff --git a/security/security.c b/security/security.c > index 2c57fe28c4f7..f7b814a3940c 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) > return 0; > } > > +/** > + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure > + * @ctx: an LSM context to be filled > + * @context: the new context value > + * @context_size: the size of the new context value > + * @id: LSM id > + * @flags: LSM defined flags > + * > + * Fill all of the fields in a user space lsm_ctx structure. > + * Caller is assumed to have verified that @ctx has enough space > + * for @context. > + * Returns 0 on success, -EFAULT on a copyout error. > + */ > +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, > + size_t context_size, u64 id, u64 flags) > +{ > + struct lsm_ctx local; > + void __user *vc = ctx; > + > + local.id = id; > + local.flags = flags; > + local.ctx_len = context_size; > + local.len = context_size + sizeof(local); > + vc += sizeof(local); > + if (copy_to_user(ctx, &local, sizeof(local))) > + return -EFAULT; > + if (context_size > 0 && copy_to_user(vc, context, context_size)) > + return -EFAULT; Can we do a single copy_to_user() call? That would avoid inconsistent user space data, could speed up a bit the operation, and make the code easier to understand. To use the stack, we need to know the maximum size of context_size for all use cases, which seems reasonable and can be checked at build time (on each LSM side, and potentially with specific context type passed as enum instead of context_size) and run time (for this generic helper). > + return 0; > +} > + > /* > * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and > * can be accessed with:
On 03/04/2023 11:47, Mickaël Salaün wrote: > > On 15/03/2023 23:47, Casey Schaufler wrote: >> Add lsm_name_to_attr(), which translates a text string to a >> LSM_ATTR value if one is available. >> >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >> the trailing attribute value. >> >> All are used in module specific components of LSM system calls. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 13 ++++++++++ >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >> security/security.c | 31 ++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+) > > [...] > >> diff --git a/security/security.c b/security/security.c >> index 2c57fe28c4f7..f7b814a3940c 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) >> return 0; >> } >> >> +/** >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >> + * @ctx: an LSM context to be filled >> + * @context: the new context value >> + * @context_size: the size of the new context value >> + * @id: LSM id >> + * @flags: LSM defined flags >> + * >> + * Fill all of the fields in a user space lsm_ctx structure. >> + * Caller is assumed to have verified that @ctx has enough space >> + * for @context. >> + * Returns 0 on success, -EFAULT on a copyout error. >> + */ >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >> + size_t context_size, u64 id, u64 flags) >> +{ >> + struct lsm_ctx local; >> + void __user *vc = ctx; >> + >> + local.id = id; >> + local.flags = flags; >> + local.ctx_len = context_size; >> + local.len = context_size + sizeof(local); >> + vc += sizeof(local); >> + if (copy_to_user(ctx, &local, sizeof(local))) >> + return -EFAULT; >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >> + return -EFAULT; > > Can we do a single copy_to_user() call? That would avoid inconsistent > user space data, could speed up a bit the operation, and make the code > easier to understand. To use the stack, we need to know the maximum size > of context_size for all use cases, which seems reasonable and can be > checked at build time (on each LSM side, and potentially with specific > context type passed as enum instead of context_size) and run time (for > this generic helper). Well, actually the context_size should be inferred from id, and the "local" size should be defined and check at build time against all context ID sizes. > > >> + return 0; >> +} >> + >> /* >> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and >> * can be accessed with:
On 03/04/2023 11:54, Mickaël Salaün wrote: > > On 03/04/2023 11:47, Mickaël Salaün wrote: >> >> On 15/03/2023 23:47, Casey Schaufler wrote: >>> Add lsm_name_to_attr(), which translates a text string to a >>> LSM_ATTR value if one is available. >>> >>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>> the trailing attribute value. >>> >>> All are used in module specific components of LSM system calls. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> include/linux/security.h | 13 ++++++++++ >>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>> security/security.c | 31 ++++++++++++++++++++++++ >>> 3 files changed, 95 insertions(+) >> >> [...] >> >>> diff --git a/security/security.c b/security/security.c >>> index 2c57fe28c4f7..f7b814a3940c 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) >>> return 0; >>> } >>> >>> +/** >>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>> + * @ctx: an LSM context to be filled >>> + * @context: the new context value >>> + * @context_size: the size of the new context value >>> + * @id: LSM id >>> + * @flags: LSM defined flags >>> + * >>> + * Fill all of the fields in a user space lsm_ctx structure. >>> + * Caller is assumed to have verified that @ctx has enough space >>> + * for @context. >>> + * Returns 0 on success, -EFAULT on a copyout error. >>> + */ >>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>> + size_t context_size, u64 id, u64 flags) >>> +{ >>> + struct lsm_ctx local; >>> + void __user *vc = ctx; >>> + >>> + local.id = id; >>> + local.flags = flags; >>> + local.ctx_len = context_size; >>> + local.len = context_size + sizeof(local); >>> + vc += sizeof(local); >>> + if (copy_to_user(ctx, &local, sizeof(local))) >>> + return -EFAULT; >>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>> + return -EFAULT; >> >> Can we do a single copy_to_user() call? That would avoid inconsistent >> user space data, could speed up a bit the operation, and make the code >> easier to understand. To use the stack, we need to know the maximum size >> of context_size for all use cases, which seems reasonable and can be >> checked at build time (on each LSM side, and potentially with specific >> context type passed as enum instead of context_size) and run time (for >> this generic helper). > > Well, actually the context_size should be inferred from id, and the > "local" size should be defined and check at build time against all > context ID sizes. @ctx_len should already be known by user space according to the LSM ID and the requested attribute. @len should already be known by user space because lsm_ctx is part of the ABI. The only reason I can think of the rationale for @len and @ctx_len is that struct lsm_ctx could gain more fields. If this happen, they would then need to be inserted before @ctx. This would make this struct lsm_ctx too flexible and complex for user space to parse correctly (e.g. for strace, gdb). I don't see where we could use @flags instead of relying on a new attribute type. I think security_getselfattr() and lsm_fill_user_ctx() could be changed to avoid each LSM to pass their own ID to lsm_fill_user_ctx(). We could have a lsm_get_attr_size(lsm_id, attr) helper (called by security_getselfattr) to group these relations, based on fixed values, exposed in the UAPI, and checked at build time with the size of the related LSM-specific attribute type. This would also allow to factor out the total size calculation needed before calling the getselfattr() implementers, and then rely on a common consistent behavior. That could also be used to not call getselfattr() implementers if they don't handle a specific attribute, and then remove their related error handling for this case. For now, the getselfattr() hook (not the related syscall) doesn't need to pass a "flags" argument to each LSM because there is no use of it. > >> >> >>> + return 0; >>> +} >>> + >>> /* >>> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and >>> * can be accessed with:
On 4/3/2023 2:47 AM, Mickaël Salaün wrote: > > On 15/03/2023 23:47, Casey Schaufler wrote: >> Add lsm_name_to_attr(), which translates a text string to a >> LSM_ATTR value if one is available. >> >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >> the trailing attribute value. >> >> All are used in module specific components of LSM system calls. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/linux/security.h | 13 ++++++++++ >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >> security/security.c | 31 ++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+) > > [...] > >> diff --git a/security/security.c b/security/security.c >> index 2c57fe28c4f7..f7b814a3940c 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >> super_block *sb) >> return 0; >> } >> +/** >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >> + * @ctx: an LSM context to be filled >> + * @context: the new context value >> + * @context_size: the size of the new context value >> + * @id: LSM id >> + * @flags: LSM defined flags >> + * >> + * Fill all of the fields in a user space lsm_ctx structure. >> + * Caller is assumed to have verified that @ctx has enough space >> + * for @context. >> + * Returns 0 on success, -EFAULT on a copyout error. >> + */ >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >> + size_t context_size, u64 id, u64 flags) >> +{ >> + struct lsm_ctx local; >> + void __user *vc = ctx; >> + >> + local.id = id; >> + local.flags = flags; >> + local.ctx_len = context_size; >> + local.len = context_size + sizeof(local); >> + vc += sizeof(local); >> + if (copy_to_user(ctx, &local, sizeof(local))) >> + return -EFAULT; >> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >> + return -EFAULT; > > Can we do a single copy_to_user() call? It would be possible, but would require allocating memory and copying the context. I don't see that as an improvement. > That would avoid inconsistent user space data, could speed up a bit > the operation, and make the code easier to understand. To use the > stack, we need to know the maximum size of context_size for all use > cases, which seems reasonable and can be checked at build time (on > each LSM side, and potentially with specific context type passed as > enum instead of context_size) and run time (for this generic helper). Knowning the maximum size of attributes for all LSMs and hard coding that here would make maintaining this code really painful. > > >> + return 0; >> +} >> + >> /* >> * The default value of the LSM hook is defined in >> linux/lsm_hook_defs.h and >> * can be accessed with:
On 4/3/2023 2:54 AM, Mickaël Salaün wrote: > > On 03/04/2023 11:47, Mickaël Salaün wrote: >> >> On 15/03/2023 23:47, Casey Schaufler wrote: >>> Add lsm_name_to_attr(), which translates a text string to a >>> LSM_ATTR value if one is available. >>> >>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>> the trailing attribute value. >>> >>> All are used in module specific components of LSM system calls. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> include/linux/security.h | 13 ++++++++++ >>> security/lsm_syscalls.c | 51 >>> ++++++++++++++++++++++++++++++++++++++++ >>> security/security.c | 31 ++++++++++++++++++++++++ >>> 3 files changed, 95 insertions(+) >> >> [...] >> >>> diff --git a/security/security.c b/security/security.c >>> index 2c57fe28c4f7..f7b814a3940c 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >>> super_block *sb) >>> return 0; >>> } >>> +/** >>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>> + * @ctx: an LSM context to be filled >>> + * @context: the new context value >>> + * @context_size: the size of the new context value >>> + * @id: LSM id >>> + * @flags: LSM defined flags >>> + * >>> + * Fill all of the fields in a user space lsm_ctx structure. >>> + * Caller is assumed to have verified that @ctx has enough space >>> + * for @context. >>> + * Returns 0 on success, -EFAULT on a copyout error. >>> + */ >>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>> + size_t context_size, u64 id, u64 flags) >>> +{ >>> + struct lsm_ctx local; >>> + void __user *vc = ctx; >>> + >>> + local.id = id; >>> + local.flags = flags; >>> + local.ctx_len = context_size; >>> + local.len = context_size + sizeof(local); >>> + vc += sizeof(local); >>> + if (copy_to_user(ctx, &local, sizeof(local))) >>> + return -EFAULT; >>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>> + return -EFAULT; >> >> Can we do a single copy_to_user() call? That would avoid inconsistent >> user space data, could speed up a bit the operation, and make the code >> easier to understand. To use the stack, we need to know the maximum size >> of context_size for all use cases, which seems reasonable and can be >> checked at build time (on each LSM side, and potentially with specific >> context type passed as enum instead of context_size) and run time (for >> this generic helper). > > Well, actually the context_size should be inferred from id, and the > "local" size should be defined and check at build time against all > context ID sizes. Again, no, I don't see this as an improvement. > >> >> >>> + return 0; >>> +} >>> + >>> /* >>> * The default value of the LSM hook is defined in >>> linux/lsm_hook_defs.h and >>> * can be accessed with:
On 03/04/2023 20:03, Casey Schaufler wrote: > On 4/3/2023 2:47 AM, Mickaël Salaün wrote: >> >> On 15/03/2023 23:47, Casey Schaufler wrote: >>> Add lsm_name_to_attr(), which translates a text string to a >>> LSM_ATTR value if one is available. >>> >>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>> the trailing attribute value. >>> >>> All are used in module specific components of LSM system calls. >>> >>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> include/linux/security.h | 13 ++++++++++ >>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++ >>> security/security.c | 31 ++++++++++++++++++++++++ >>> 3 files changed, 95 insertions(+) >> >> [...] >> >>> diff --git a/security/security.c b/security/security.c >>> index 2c57fe28c4f7..f7b814a3940c 100644 >>> --- a/security/security.c >>> +++ b/security/security.c >>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >>> super_block *sb) >>> return 0; >>> } >>> +/** >>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>> + * @ctx: an LSM context to be filled >>> + * @context: the new context value >>> + * @context_size: the size of the new context value >>> + * @id: LSM id >>> + * @flags: LSM defined flags >>> + * >>> + * Fill all of the fields in a user space lsm_ctx structure. >>> + * Caller is assumed to have verified that @ctx has enough space >>> + * for @context. >>> + * Returns 0 on success, -EFAULT on a copyout error. >>> + */ >>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>> + size_t context_size, u64 id, u64 flags) >>> +{ >>> + struct lsm_ctx local; >>> + void __user *vc = ctx; >>> + >>> + local.id = id; >>> + local.flags = flags; >>> + local.ctx_len = context_size; >>> + local.len = context_size + sizeof(local); >>> + vc += sizeof(local); >>> + if (copy_to_user(ctx, &local, sizeof(local))) >>> + return -EFAULT; >>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>> + return -EFAULT; >> >> Can we do a single copy_to_user() call? > > It would be possible, but would require allocating memory and copying > the context. I don't see that as an improvement. > >> That would avoid inconsistent user space data, could speed up a bit >> the operation, and make the code easier to understand. To use the >> stack, we need to know the maximum size of context_size for all use >> cases, which seems reasonable and can be checked at build time (on >> each LSM side, and potentially with specific context type passed as >> enum instead of context_size) and run time (for this generic helper). > > Knowning the maximum size of attributes for all LSMs and hard coding > that here would make maintaining this code really painful. Hmm, I forgot about variable-length strings, but maybe a reasonable common maximum size (that could fit on the stack) could be found? > >> >> >>> + return 0; >>> +} >>> + >>> /* >>> * The default value of the LSM hook is defined in >>> linux/lsm_hook_defs.h and >>> * can be accessed with:
On 4/3/2023 11:06 AM, Mickaël Salaün wrote: > > On 03/04/2023 20:03, Casey Schaufler wrote: >> On 4/3/2023 2:47 AM, Mickaël Salaün wrote: >>> >>> On 15/03/2023 23:47, Casey Schaufler wrote: >>>> Add lsm_name_to_attr(), which translates a text string to a >>>> LSM_ATTR value if one is available. >>>> >>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including >>>> the trailing attribute value. >>>> >>>> All are used in module specific components of LSM system calls. >>>> >>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >>>> --- >>>> include/linux/security.h | 13 ++++++++++ >>>> security/lsm_syscalls.c | 51 >>>> ++++++++++++++++++++++++++++++++++++++++ >>>> security/security.c | 31 ++++++++++++++++++++++++ >>>> 3 files changed, 95 insertions(+) >>> >>> [...] >>> >>>> diff --git a/security/security.c b/security/security.c >>>> index 2c57fe28c4f7..f7b814a3940c 100644 >>>> --- a/security/security.c >>>> +++ b/security/security.c >>>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct >>>> super_block *sb) >>>> return 0; >>>> } >>>> +/** >>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure >>>> + * @ctx: an LSM context to be filled >>>> + * @context: the new context value >>>> + * @context_size: the size of the new context value >>>> + * @id: LSM id >>>> + * @flags: LSM defined flags >>>> + * >>>> + * Fill all of the fields in a user space lsm_ctx structure. >>>> + * Caller is assumed to have verified that @ctx has enough space >>>> + * for @context. >>>> + * Returns 0 on success, -EFAULT on a copyout error. >>>> + */ >>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, >>>> + size_t context_size, u64 id, u64 flags) >>>> +{ >>>> + struct lsm_ctx local; >>>> + void __user *vc = ctx; >>>> + >>>> + local.id = id; >>>> + local.flags = flags; >>>> + local.ctx_len = context_size; >>>> + local.len = context_size + sizeof(local); >>>> + vc += sizeof(local); >>>> + if (copy_to_user(ctx, &local, sizeof(local))) >>>> + return -EFAULT; >>>> + if (context_size > 0 && copy_to_user(vc, context, context_size)) >>>> + return -EFAULT; >>> >>> Can we do a single copy_to_user() call? >> >> It would be possible, but would require allocating memory and copying >> the context. I don't see that as an improvement. >> >>> That would avoid inconsistent user space data, could speed up a bit >>> the operation, and make the code easier to understand. To use the >>> stack, we need to know the maximum size of context_size for all use >>> cases, which seems reasonable and can be checked at build time (on >>> each LSM side, and potentially with specific context type passed as >>> enum instead of context_size) and run time (for this generic helper). >> >> Knowning the maximum size of attributes for all LSMs and hard coding >> that here would make maintaining this code really painful. > > Hmm, I forgot about variable-length strings, but maybe a reasonable > common maximum size (that could fit on the stack) could be found? Putting a maximum size limit on LSM attributes just to reduce the number of copy_to_user() calls in this helper function does not make a whole lot of sense to me. > >> >>> >>> >>>> + return 0; >>>> +} >>>> + >>>> /* >>>> * The default value of the LSM hook is defined in >>>> linux/lsm_hook_defs.h and >>>> * can be accessed with:
diff --git a/include/linux/security.h b/include/linux/security.h index 329cd9d2be50..a5e860d332b5 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -263,6 +263,7 @@ int unregister_blocking_lsm_notifier(struct notifier_block *nb); /* prototypes */ extern int security_init(void); extern int early_security_init(void); +extern u64 lsm_name_to_attr(const char *name); /* Security operations */ int security_binder_set_context_mgr(const struct cred *mgr); @@ -491,6 +492,8 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen); int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); int security_locked_down(enum lockdown_reason what); +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, + size_t context_size, u64 id, u64 flags); #else /* CONFIG_SECURITY */ static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data) @@ -508,6 +511,11 @@ static inline int unregister_blocking_lsm_notifier(struct notifier_block *nb) return 0; } +static inline u64 lsm_name_to_attr(const char *name) +{ + return 0; +} + static inline void security_free_mnt_opts(void **mnt_opts) { } @@ -1420,6 +1428,11 @@ static inline int security_locked_down(enum lockdown_reason what) { return 0; } +static inline int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, + size_t context_size, u64 id, u64 flags) +{ + return 0; +} #endif /* CONFIG_SECURITY */ #if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE) diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c index 6efbe244d304..55d849ad5d6e 100644 --- a/security/lsm_syscalls.c +++ b/security/lsm_syscalls.c @@ -17,6 +17,57 @@ #include <linux/lsm_hooks.h> #include <uapi/linux/lsm.h> +struct attr_map { + char *name; + u64 attr; +}; + +static const struct attr_map lsm_attr_names[] = { + { + .name = "current", + .attr = LSM_ATTR_CURRENT, + }, + { + .name = "exec", + .attr = LSM_ATTR_EXEC, + }, + { + .name = "fscreate", + .attr = LSM_ATTR_FSCREATE, + }, + { + .name = "keycreate", + .attr = LSM_ATTR_KEYCREATE, + }, + { + .name = "prev", + .attr = LSM_ATTR_PREV, + }, + { + .name = "sockcreate", + .attr = LSM_ATTR_SOCKCREATE, + }, +}; + +/** + * lsm_name_to_attr - map an LSM attribute name to its ID + * @name: name of the attribute + * + * Look the given @name up in the table of know attribute names. + * + * Returns the LSM attribute value associated with @name, or 0 if + * there is no mapping. + */ +u64 lsm_name_to_attr(const char *name) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) + if (!strcmp(name, lsm_attr_names[i].name)) + return lsm_attr_names[i].attr; + return 0; +} + /** * sys_lsm_set_self_attr - Set current task's security module attribute * @attr: which attribute to set diff --git a/security/security.c b/security/security.c index 2c57fe28c4f7..f7b814a3940c 100644 --- a/security/security.c +++ b/security/security.c @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb) return 0; } +/** + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure + * @ctx: an LSM context to be filled + * @context: the new context value + * @context_size: the size of the new context value + * @id: LSM id + * @flags: LSM defined flags + * + * Fill all of the fields in a user space lsm_ctx structure. + * Caller is assumed to have verified that @ctx has enough space + * for @context. + * Returns 0 on success, -EFAULT on a copyout error. + */ +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context, + size_t context_size, u64 id, u64 flags) +{ + struct lsm_ctx local; + void __user *vc = ctx; + + local.id = id; + local.flags = flags; + local.ctx_len = context_size; + local.len = context_size + sizeof(local); + vc += sizeof(local); + if (copy_to_user(ctx, &local, sizeof(local))) + return -EFAULT; + if (context_size > 0 && copy_to_user(vc, context, context_size)) + return -EFAULT; + return 0; +} + /* * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and * can be accessed with: