Message ID | 20230109180717.58855-5-casey@schaufler-ca.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp2301193wrt; Mon, 9 Jan 2023 10:12:22 -0800 (PST) X-Google-Smtp-Source: AMrXdXteLMRHGc3IYB9B5aVJehy+r41rzzWwXFhHOkd8wLOJh7ZwamaUWxb3ARB0KXu92G/I+324 X-Received: by 2002:a17:90a:470d:b0:219:65b0:9932 with SMTP id h13-20020a17090a470d00b0021965b09932mr66546338pjg.1.1673287942476; Mon, 09 Jan 2023 10:12:22 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1673287942; cv=none; d=google.com; s=arc-20160816; b=NRg/CZll+oV3Enh5tNCEXmB7Vk8oHF1SF+yFgGHJBuT1uuNA4fgymi3hPpjh/F4jsT wffbxzrhQniJRqvkxe+NvO91cxsKFjJgLaFsil0CZ/KYOp9exqL1t2ClQlSnU2SdOZE4 os2EHdtzSD9e5gyQ+26ddDvKtfmTFV/pfl77V/1wJ2DwXDJh0YH13MMnHAZSIiUP1LLU 033bTX+zd0bBZY1t888QTO+Ta3NTMI4GQ/O0eaZtRzoxgb1gsC+KjMGG5dili/xAZQLN H27IPXKlKhI2E2H+FhWrfEifiRv/4ljohcC76QFsZN7/CGddWHHAVaeUixiEcD/3Y5bD 5hGQ== 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=UBknTJ5AK+8ibzgjqpdN5Pyy2eZ6OEF4EO5PatESuek=; b=KOLJrDk9W91FFYjnXbcOrSYpf+cIRi83PV4K8qbKX8qubNme7amXxgwaBDVFLxIjtS ecZaP+ENPoAjbPwhzmMscN0pEMMl9GOQdcpNdB/frlBOkyD6h32fYRhHhkX3Kl4lp0BX HX9WUnGlPzgWGCfDDAc4XnoBcUcsOu8vqsPOa6dAXRgZFopR4Kf/MZ66vchsLV1aNL0I +oavRVJw1SeyXZ2iTT2UltwGuQYPgYCEVBo5rt8ybQ9/nTn9ispAwuJFIfFzm+ITlbiL VHeXpfjEQUvVVM6bnrWWrAe0JIaz34N4I7iidwFIn5ocWUF4ViiLHsVgi4qLIylOd4Q+ 6bFw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@yahoo.com header.s=s2048 header.b=DfByM41f; 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 pj14-20020a17090b4f4e00b00218a7391526si10041174pjb.186.2023.01.09.10.12.09; Mon, 09 Jan 2023 10:12:22 -0800 (PST) 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=DfByM41f; 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 S237573AbjAISK6 (ORCPT <rfc822;syz17693488234@gmail.com> + 99 others); Mon, 9 Jan 2023 13:10:58 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58356 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237428AbjAISJn (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 9 Jan 2023 13:09:43 -0500 Received: from sonic310-30.consmr.mail.ne1.yahoo.com (sonic310-30.consmr.mail.ne1.yahoo.com [66.163.186.211]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CE89613D68 for <linux-kernel@vger.kernel.org>; Mon, 9 Jan 2023 10:09:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1673287749; bh=UBknTJ5AK+8ibzgjqpdN5Pyy2eZ6OEF4EO5PatESuek=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From:Subject:Reply-To; b=DfByM41fmvEA7yAHybBgMe2o8SKJ57dXOHxCV6hDe8p1sqypJnuF2fEqwIJ89HUmKIVadB8QygYPduw2dB/rqS3o7QzZCj2bIBz5NFCa4Qcb8mayoR0ci5yaa8fErkAgcaihnuoVU36elCPPZlSN+86ZeJU8/fuleuIvQiIMrgiVymHX8WwlU9azU1XaWjLs1JCIk+3v3Zn/rRV1lhMu3jQHCaapvABzLqBoVWFcI7MTkG6giyRDXUJD5P+RH7keJU5jHVnPUqyoFR2Q2YwmXpA4uMBaZ0MQr/1bHRZH1r8dDQz47EsFmqTkaDC4YATTIV4dv9gzwTQIJerx3m9O5g== X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1673287749; bh=4qqI7rhC/nebP38HB3U/gZfWNnaDfbZTPFeL34H19K+=; h=X-Sonic-MF:From:To:Subject:Date:From:Subject; b=CRJ+sGP+mWQPm4CQOHj5OUlTOcpU3wCkgLtfOkT6Bc67ocglouhiH5lxLLQsreSSvBak0qyohBczuuFJ0AEYSTMiTKUk9gJmYV/7VXAKT06ziVYQTcY3siJhOg8bp2qiKv/4fyzuhv//avharsmVtZGxr85VYkXRpwjqq7oNgF5eWbfi1k9btRCNOBCuQ9xq+OporicOeH/1FqGfa2+MnlNzeUkJix0hkuD3mB8Rqc1FlMcjj50lOJ1Qjik97UDLNq5yUsOg/BRHGOg9jvlIFUZXjpvKHuspjCTt463e/ttyvk8RuZ3ACQQ3XicYOuGYLzmgfOqj3G6lZDfteEkVwA== X-YMail-OSG: 3DBtMP8VM1kdXlwuUwT2CZ.P8.UnM3dGO00k_f.zL7jntUkaFYmruyIITd1F1VK Bit2sfG.SJZlkf3dTWGCG0dGeLJw_E96yusRDDBhTiFP7fr8_bX6YSD1_czQW7s.MDSz7yBqrgPD Zjnr_EAJ2OfXm4CaJVt5dc75Id5axvdBSfCn1tWb9LTLnzwS1Lx9iej7dqrTV3DIQcLPXS3Uhwa3 ZhOrIU9RkgNTq0ByBujqREq_2TrrLpBH_ZEBR6pCz1qpEzSo0M.KNFzCcPn8TD.Hfv4ZCv2sXXQl XRuTvhq0Jal25ekNit0czWzgW89ZISJjSdEiwdjymQ6n34SKJ2nKuvODbjB.Mly7AU0ogkTahmKd RQ2OvxezkWzg14JSVUa07tKpFEkxn6EoRpCQQigju.VAwEJa6YjauHE0f6l_6u3g7hGYWSdqwp0I crxBRs_lnr8I9j2h4eEnIcHbVDqJSxp1gzUjZAD7xK.5CFCDnOjxNIhbbGJd9o8Qu616JBMTEPHU _YxlBQHHEozCaAMzAs68Yp.eI0zAhs9g6M4aDWAiu7RDepCW_NPiuP0.LTYBEWHAtzZzDUe4a_VA VnQeTPkj5ZM.lqpxc9jOkGOt8if4Pu0vAQ8b7Tou5c8prubRhRwqGXpgorVFeCCmuS5SKrG3aeKw tw5SkGF77iYcii.egdeABh8C8IlEzMvg4.SLjgcK.fQ5USJEpt0fMW26z5kM_kawZxDx3PHhRCL3 32zzuE_af45Y.0VldvZ5ctbjqNLRTIcMqwQUQ2RXAgvpViSWOPPCjbDoeUlfNDsDIT86Um84ie3t HmZ5ZmRtA.vTfe1WXX9evXCQMyhytTSmuNibDktMjiNO.9RFilY76ovO0yTK6ts7K7wtboRpI0Y5 7KQzezK19J3OjatzRT2Y6HLFz0EN6H3lYHK_bVnuYDlucQsNlryxYWBZhzuX6meq0XltkbEd7.Dg EWPJOqMydDcjOg2iFIuw0IQsV_5YD06OhwwCRreOwKnJTnuiMC2XDPmoTEOkaZgOMX97PQ.znR4C ZL0J9oAofCpAX8U3XCCUWvSr2O_sSDUhkkk4VY.JzU_e9GLKkPyo7V0k0Atio6Tm4tl4fWzGz4zE k_8UDjKX944ytNgXOZE_DvloNbOXUg60lh3pZ8g.Yx8CYor.xSNAVh44tRsb1rQkuE_c.mqrDzvR Q4n2DwImAllbRgPK71W7jPxC30v1zrKUp01FDkUraZSSdJa8Km1TlpyD.gluHBw2Tuj83oSFRmTa rYVJbz7gNRVhUxL1R3kfvWzqGHezoRYpXjJ4Ud2kFdKZ0WFKX9lUDkMAivOJg61JyYO8gYsDz7.M 17MrakG5fbJ67qm5ULpBMxPVcYv_mYbGPZYuH0RET7OCTwPgFUMm6sZuwrRQ.sDVQcqFTiMt9PVI 9qNMhkxjJMFc2r9OnzrTRmjBFKcdUOe1z4pfSmc3pDfRUNBDRz9WfpypdumG.uGmrhKrw_kFrAMX wCY64WbDr3L6QnmzeK2imbA6WQCBkMK_eM9TcswqWdqM4b6OI85fokFlr0s3C.bpLzD6AZs.LAH6 6LATwlIEN9Zxb_gd95ai7QErI7BOM2k1U5DNeHTGQ4qKyz97zwWxsBYFKT9phi5_Oiefpl9Wtp85 Q7UhMDU0cio1ECxAxqSYgLlDklKQoMQcKka8c4nTxlLsn.9mOnCpy04oI5__qRDv6Ss8drfFC7bc JBRR3ETJ5Lbb8MZj1AgoauhPNc0xDyHzkkuuWs4f3Ti5B902RhQkar3WR5gUIIlNkx9g.C1ZA.0f XpVUcmsRunZ9.Y.Q3dkzm3R9Ea7eW.HyT5.cMybnCVriJ3liFQ_NIkDdt8H2gznqfnciAxaTmjdM _KGHGtQSkTMvN9MZme0GcAUHvz7bAn4.WDUx0nElhpe99JrtzVsi.63wVBkZoHxg_v7zZO21KozT y3jn1yaf7VxuuO50HvHrIHF4DfS31grfKBv3SyB8UAuwZmDFTRnpf_yImygpyNTeo8WGzjTPU.Yt GFAShQmxU4ADL90l_eWBZRWkqS_f1PZvvqddW4q07Y7kj6c1YdZ1JvQHVIN0aN0zylMCcodgM9og 5Bk0YZ_lnJhpaywd.mOyt1zx6gpTpVKpFAn41ciF3ys9WLt1nOo3pEskSFzT3JQxSsxK.WEpUqdp QTFCG8_f_UYTmFHWySA8N4vru2bkrG0nbEoonCB2zo1YzB8Ac.Jt2zk6.JenEmNfpHnP3JMStCUx ghVqPzlkQLQYkSYgXFITlUUrjTshU5uUWmt372cvj3UdQBWlz90EjUS8ULSLcdNyX5.NsXdRCcSh FdE3jt.CIJCpZY1QlvV0bgvE- X-Sonic-MF: <casey@schaufler-ca.com> Received: from sonic.gate.mail.ne1.yahoo.com by sonic310.consmr.mail.ne1.yahoo.com with HTTP; Mon, 9 Jan 2023 18:09:09 +0000 Received: by hermes--production-ne1-7b69748c4d-474lb (Yahoo Inc. Hermes SMTP Server) with ESMTPA ID 3be57d1d3a3d9a781b586d3784815f58; Mon, 09 Jan 2023 18:09:03 +0000 (UTC) From: Casey Schaufler <casey@schaufler-ca.com> To: casey.schaufler@intel.com, paul@paul-moore.com, linux-security-module@vger.kernel.org Cc: casey@schaufler-ca.com, 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 v5 4/8] LSM: lsm_get_self_attr syscall for LSM self attributes Date: Mon, 9 Jan 2023 10:07:13 -0800 Message-Id: <20230109180717.58855-5-casey@schaufler-ca.com> X-Mailer: git-send-email 2.39.0 In-Reply-To: <20230109180717.58855-1-casey@schaufler-ca.com> References: <20230109180717.58855-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 autolearn=unavailable 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?1754569577371683491?= X-GMAIL-MSGID: =?utf-8?q?1754569577371683491?= |
Series |
LSM: Three basic syscalls
|
|
Commit Message
Casey Schaufler
Jan. 9, 2023, 6:07 p.m. UTC
Create a system call lsm_get_self_attr() to provide the security
module maintained attributes of the current process. Historically
these attributes have been exposed to user space via entries in
procfs under /proc/self/attr.
Attributes are provided as a collection of lsm_ctx structures
which are placed into a user supplied buffer. Each structure
identifys the size of the attribute, and the attribute value.
The format of the attribute value is defined by the security
module, but will always be \0 terminated. The ctx_len value
will always be strlen(ctx)+1.
---------------------------
| __u32 id |
---------------------------
| __u64 flags |
---------------------------
| __kernel_size_t ctx_len |
---------------------------
| __u8 ctx[ctx_len] |
---------------------------
| __u32 id |
---------------------------
| __u64 flags |
---------------------------
| __kernel_size_t ctx_len |
---------------------------
| __u8 ctx[ctx_len] |
---------------------------
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
Documentation/userspace-api/lsm.rst | 9 ++
include/linux/syscalls.h | 3 +
include/uapi/linux/lsm.h | 21 ++++
kernel/sys_ni.c | 3 +
security/Makefile | 1 +
security/lsm_syscalls.c | 182 ++++++++++++++++++++++++++++
6 files changed, 219 insertions(+)
create mode 100644 security/lsm_syscalls.c
Comments
On Mon, Jan 9, 2023 at 1:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > Create a system call lsm_get_self_attr() to provide the security > module maintained attributes of the current process. Historically > these attributes have been exposed to user space via entries in > procfs under /proc/self/attr. > > Attributes are provided as a collection of lsm_ctx structures > which are placed into a user supplied buffer. Each structure > identifys the size of the attribute, and the attribute value. ^^^ identifies > The format of the attribute value is defined by the security > module, but will always be \0 terminated. The ctx_len value > will always be strlen(ctx)+1. I don't want to limit ourselves to only sending string values as attributes as who knows what we might need to do in the future, and the struct was originally designed to support both strings and binary data. I would suggest changing the sentences above to something like this: The format of the attribute value is defined by the individual LSM, with the attribute itself stored in @ctx and the length of the attribute stored in @ctx_len. Both strings and arbitrary binary attributes are supported, but strings should be NULL terminated and @ctx_len should be equal to `strlen(@ctx) + 1`. > --------------------------- > | __u32 id | > --------------------------- > | __u64 flags | > --------------------------- > | __kernel_size_t ctx_len | > --------------------------- > | __u8 ctx[ctx_len] | > --------------------------- > | __u32 id | > --------------------------- > | __u64 flags | > --------------------------- > | __kernel_size_t ctx_len | > --------------------------- > | __u8 ctx[ctx_len] | > --------------------------- Don't repeat the structure layout in memory twice here, it's confusing. I also think it would be easier to read, and arguably more useful, to simply copy the struct definition into the description instead of the ASCII art column. Although, this has got me wondering if we should think about aligning the lsm_ctx structs when we are populating them in the kernel; more on this below ... > --- > Documentation/userspace-api/lsm.rst | 9 ++ > include/linux/syscalls.h | 3 + > include/uapi/linux/lsm.h | 21 ++++ > kernel/sys_ni.c | 3 + > security/Makefile | 1 + > security/lsm_syscalls.c | 182 ++++++++++++++++++++++++++++ > 6 files changed, 219 insertions(+) > create mode 100644 security/lsm_syscalls.c ... > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h > index 33a0ee3bcb2e..a89205c70ffa 100644 > --- a/include/linux/syscalls.h > +++ b/include/linux/syscalls.h > @@ -71,6 +71,7 @@ struct clone_args; > struct open_how; > struct mount_attr; > struct landlock_ruleset_attr; > +struct lsm_ctx; > enum landlock_rule_type; > > #include <linux/types.h> > @@ -1058,6 +1059,8 @@ asmlinkage long sys_memfd_secret(unsigned int flags); > asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len, > unsigned long home_node, > unsigned long flags); > +asmlinkage long sys_lsm_get_self_attr(struct lsm_ctx *ctx, size_t *size, > + int flags); > > /* > * Architecture-specific system calls > diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h > index 61a91b7d946f..8674d8c6b326 100644 > --- a/include/uapi/linux/lsm.h > +++ b/include/uapi/linux/lsm.h > @@ -9,6 +9,27 @@ > #ifndef _UAPI_LINUX_LSM_H > #define _UAPI_LINUX_LSM_H > > +#include <linux/types.h> > +#include <linux/unistd.h> > + > +/** > + * struct lsm_ctx - LSM context > + * @id: the LSM id number, see LSM_ID_XXX As mentioned above, it occurred to me that we might want want to pad out the lsm_ctx struct to ensure that the "array" of lsm_ctx is nicely aligned. I know some systems used to complain about unaligned accesses, and even on those that don't complain tend to be faster when access are aligned. We can either implicitly align the individual lsm_ctx structs or we can add a total length field (in addition to the @ctx_len field) so that the padding/alignment is explicit. Adding an explicit total length field could have some other advantages in that it, in conjunction with the existing @flags field, would allow an individual LSM to "extend" the lsm_ctx struct to provide additional LSM specific information in the case where the single @ctx field was not sufficient. Think of it as some additional future proofing in addition to explicit padding. > + * @flags: context specifier and LSM specific flags * @flags: LSM specific flags Only the individual LSM specified in @id should ever interpret @flags or @ctx. > + * @ctx_len: the size of @ctx > + * @ctx: the LSM context, a nul terminated string * @ctx: the LSM context value > + * @ctx in a nul terminated string. > + * (strlen(@ctx) < @ctx_len) is always true. > + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed. > + */ Let's rework the extra description too based on the comments above. For the sake of clarity, here is what I'm currently thinking (comments and feedback are encouraged): /** * struct lsm_ctx - LSM context information * @id: the LSM ID token, see LSM_ID_XXX * @flags: LSM specific flags * @len: length of the lsm_ctx struct + extra (?) + padding * @ctx_len: the size of @ctx * @ctx: the LSM context value * * The @len field MUST be equal to size of the lsm_ctx struct * plus any additional padding and/or data placed after @ctx. * * In all cases @ctx_len MUST be equal to length of @ctx. If * @ctx is a string value, it should be nul terminated with * @ctx_len equal to `strlen(@ctx) + 1`. Binary @ctx values * are supported. * * The @flags and @ctx fields SHOULD only be interpreted by the * LSM specified by @id; they MUST be set to zero/0 when not used. */ struct lsm_ctx { __u32 id; __u64 flags; __kernel_size_t len; __kernel_size_t ctx_len; __u8 ctx[]; }; > diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > new file mode 100644 > index 000000000000..55e8bf61ac8a > --- /dev/null > +++ b/security/lsm_syscalls.c > @@ -0,0 +1,182 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * System calls implementing the Linux Security Module API. > + * > + * Copyright (C) 2022 Casey Schaufler <casey@schaufler-ca.com> > + * Copyright (C) 2022 Intel Corporation > + */ > + > +#include <asm/current.h> > +#include <linux/compiler_types.h> > +#include <linux/err.h> > +#include <linux/errno.h> > +#include <linux/security.h> > +#include <linux/stddef.h> > +#include <linux/syscalls.h> > +#include <linux/types.h> > +#include <linux/lsm_hooks.h> > +#include <uapi/linux/lsm.h> > + > +struct attrs_used_map { > + char *name; > + int attrs_used; Based on the usage below it really seems like @attrs_used should just be @attr, yes? That said, I'm not too bothered by it either way so if you really love @attrs_used that's fine. > +}; > + > +static const struct attrs_used_map lsm_attr_names[] = { We can probably just call this "attr_map" right? I mean the "used" portion is pretty inherent in the fact that we're defining a mapping :) > + { .name = "current", .attrs_used = LSM_ATTR_CURRENT, }, > + { .name = "exec", .attrs_used = LSM_ATTR_EXEC, }, > + { .name = "fscreate", .attrs_used = LSM_ATTR_FSCREATE, }, > + { .name = "keycreate", .attrs_used = LSM_ATTR_KEYCREATE, }, > + { .name = "prev", .attrs_used = LSM_ATTR_PREV, }, > + { .name = "sockcreate", .attrs_used = LSM_ATTR_SOCKCREATE, }, > +}; > + > +static int attr_used_index(u32 flags) Since you can only return one index value at a time in this function you can't really support multiple attribute bits set in the @flags parameter so why not change the prototype to better match the required usage, example: static int attr_index(u32 attr) > +{ > + int i; > + > + if (flags == 0) > + return -EINVAL; > + > + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > + if ((lsm_attr_names[i].attrs_used & flags) == flags) > + return i; Given the above, why not simplify the above test to this: if (lsm_attr_name[i].attr == attr) return i; If we don't care about failing fast in the case of being passed 0 (why would we?) we can define this function as follows: static int attr_index(u32 attr) { int i; for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) if (lsm_attr_names[i].attr == attr) return i; return -EINVAL; } If we wanted to streamline things even further we could define attr_map a bit differently and drop the loop in attr_index(). Yes, it does waste attr_map[0], but I don't think anyone is going to be too upset about one wasted index if it scales better. static const struct attr_map[] = { [LSM_ATTR_CURRENT] = { .name = "current", .attr = LSM_ATTR_CURRENT }, [LSM_ATTR_EXEC] = { .name = "exec", .attr = LSM_ATTR_EXEC }, ... }; static int attr_index(u32 attr) { if (attr == 0 || attr >= ARRAY_SIZE(attr_map)) return -EINVAL; return attr; } If you did this you could probably also convert attr_map from a struct to a simple array of strings as the attribute value would be the associated index. > +/** > + * sys_lsm_get_self_attr - Return current task's security module attributes > + * @ctx: the LSM contexts > + * @size: size of @ctx, updated on return > + * @flags: which attribute to return > + * > + * Returns the calling task's LSM contexts. On success this > + * function returns the number of @ctx array elements. This value > + * may be zero if there are no LSM contexts assigned. If @size is > + * insufficient to contain the return data -E2BIG is returned and > + * @size is set to the minimum required size. In all other cases > + * a negative value indicating the error is returned. > + */ > +SYSCALL_DEFINE3(lsm_get_self_attr, > + struct lsm_ctx __user *, ctx, > + size_t __user *, size, > + u32, flags) > +{ > + int i; > + int rc = 0; > + int len; > + int attr; > + int count = 0; > + void *curr; > + char *cp; > + char *np; > + char **interum_ctx; > + size_t total_size = 0; > + struct lsm_ctx *ip; > + struct lsm_ctx *interum; > + struct lsm_ctx *final = NULL; > + > + attr = attr_used_index(flags); > + if (attr < 0) > + return attr; > + > + interum = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * > + sizeof(*interum), GFP_KERNEL); > + if (interum == NULL) > + return -ENOMEM; > + ip = interum; > + > + interum_ctx = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * > + sizeof(*interum_ctx), GFP_KERNEL); > + if (interum_ctx == NULL) { > + kfree(interum); > + return -ENOMEM; > + } > + > + for (i = 0; i < lsm_active_cnt; i++) { > + if ((lsm_idlist[i]->attrs_used & > + lsm_attr_names[attr].attrs_used) == 0) > + continue; > + > + len = security_getprocattr(current, lsm_idlist[i]->id, > + lsm_attr_names[attr].name, > + &cp); > + if (len <= 0) > + continue; > + > + ip->id = lsm_idlist[i]->id; > + ip->flags = lsm_attr_names[attr].attrs_used; > + interum_ctx[count] = cp; > + > + /* > + * A security module that returns a binary attribute > + * will need to identify itself to prevent string > + * processing. > + * > + * At least one security module adds a \n at the > + * end of a context to make it look nicer. Change > + * that to a \0 so that user space doesn't have to > + * work around it. > + * > + * Security modules have been inconsistent about > + * including the \0 terminator in the size. If it's > + * not there make space for it. > + * > + * The length returned will reflect the length of > + * the string provided by the security module, which > + * may not match what getprocattr returned. > + */ > + np = strnchr(cp, len, '\n'); > + if (np != NULL) > + *np = '\0'; > + ip->ctx_len = strnlen(cp, len) + 1; > + total_size += sizeof(*interum) + ip->ctx_len; > + ip++; > + count++; > + } > + > + if (count == 0) > + goto free_out; > + > + final = kzalloc(total_size, GFP_KERNEL); > + if (final == NULL) { > + rc = -ENOMEM; > + goto free_out; > + } > + > + curr = final; > + ip = interum; > + for (i = 0; i < count; i++) { > + memcpy(curr, ip, sizeof(*interum)); > + curr += sizeof(*interum); > + if (ip->ctx_len > 1) > + memcpy(curr, interum_ctx[i], ip->ctx_len - 1); > + curr += ip->ctx_len; > + ip++; > + } > + > + if (get_user(len, size)) { > + rc = -EFAULT; > + goto free_out; > + } > + if (total_size > len) { > + rc = -ERANGE; > + if (put_user(total_size, size) != 0) > + rc = -EFAULT; > + goto free_out; > + } > + if (copy_to_user(ctx, final, total_size) != 0 || > + put_user(total_size, size) != 0) > + rc = -EFAULT; > + else > + rc = count; > + > +free_out: > + for (i = 0; i < count; i++) > + kfree(interum_ctx[i]); > + kfree(interum_ctx); > + kfree(interum); > + kfree(final); > + return rc; > +} Hmm. That's all kinda painful isn't it? I think trying to reuse security_getprocattr() is doing more harm than good with all the awkward handling necessary to ensure consistent output. While it's nice to be able to reuse existing interfaces, one of the main motivations behind the LSM syscall effort is to create a cleaner interface that was designed from the beginning to support multiple LSMs and provide a level of extensibility that we do not currently have with the procfs interface. Hacking together all our old crap to make this happen seems very wrong to me. With that in mind I would like to propose we introduce a new LSM hook to populate a lsm_ctx struct based on a LSM_ATTR_XXX value: int security_sys_getselfattr(u64 attr, struct lsm_ctx __user *ctx, size_t *len); The individual LSMs would be responsible for fully populating their lsm_ctx struct specified by @ctx (note the __user tagging) and would return 0 on success or negative values on failure. The maximum size of the @ctx buffer would be passed in via @len and the used size would be returned in @len; in the case of an too-small @ctx, -E2BIG would be returned and the necessary size would be returned in @len (just as discussed for the syscall itself). This way the LSM layer syscall function would not need to worry about properly terminating the lsm_ctx::ctx field, setting any LSM specific flags, etc. Passing the __user pointer directly not only greatly simplifies the LSM layer, it also has the potential to reduce the number of allocations/copies. Taking this approach should shrink the LSM layer syscall function to simply needing to validate the passed @flags before looping through the LSMs calling security_sys_getselfattr(). The lsm_ctx pointer would need to be incremented appropriately for each call, and a total length/size count would need to be maintained in case the buffer is too small, but those should be relatively minor things. -- paul-moore.com
On 1/11/2023 1:07 PM, Paul Moore wrote: > On Mon, Jan 9, 2023 at 1:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> Create a system call lsm_get_self_attr() to provide the security >> module maintained attributes of the current process. Historically >> these attributes have been exposed to user space via entries in >> procfs under /proc/self/attr. >> >> Attributes are provided as a collection of lsm_ctx structures >> which are placed into a user supplied buffer. Each structure >> identifys the size of the attribute, and the attribute value. > ^^^ > identifies Pluralses are hard in the vicinity of 'y's. ;) >> The format of the attribute value is defined by the security >> module, but will always be \0 terminated. The ctx_len value >> will always be strlen(ctx)+1. > I don't want to limit ourselves to only sending string values as > attributes as who knows what we might need to do in the future, and > the struct was originally designed to support both strings and binary > data. I would suggest changing the sentences above to something like > this: Part of creating a sane and sensible API is setting rational limitations. While a "context" has always allowed for a binary value it has always been a user friendly, nul terminated string. The one case where someone proposed otherwise was my "hideous" format for compound contexts, and we know how well that was received. If we're serious about cleaning up our API let's quit bending over to support something no one is using and that we'd prefer they didn't. That's my preference. Is there anyone who wants a binary interface, or any really good reason to provide one? I'm not going to stand fast on a strings only interface, but it would make it significantly cleaner if we had one. > > The format of the attribute value is defined by the individual LSM, > with the attribute itself stored in @ctx and the length of the > attribute stored in @ctx_len. Both strings and arbitrary binary > attributes are supported, but strings should be NULL terminated and > @ctx_len should be equal to `strlen(@ctx) + 1`. > >> --------------------------- >> | __u32 id | >> --------------------------- >> | __u64 flags | >> --------------------------- >> | __kernel_size_t ctx_len | >> --------------------------- >> | __u8 ctx[ctx_len] | >> --------------------------- >> | __u32 id | >> --------------------------- >> | __u64 flags | >> --------------------------- >> | __kernel_size_t ctx_len | >> --------------------------- >> | __u8 ctx[ctx_len] | >> --------------------------- > Don't repeat the structure layout in memory twice here, it's > confusing. I also think it would be easier to read, and arguably more > useful, to simply copy the struct definition into the description > instead of the ASCII art column. OK on both. > Although, this has got me wondering if we should think about aligning > the lsm_ctx structs when we are populating them in the kernel; more on > this below ... As you say below, we'll need a total_len for this, but OK. >> --- >> Documentation/userspace-api/lsm.rst | 9 ++ >> include/linux/syscalls.h | 3 + >> include/uapi/linux/lsm.h | 21 ++++ >> kernel/sys_ni.c | 3 + >> security/Makefile | 1 + >> security/lsm_syscalls.c | 182 ++++++++++++++++++++++++++++ >> 6 files changed, 219 insertions(+) >> create mode 100644 security/lsm_syscalls.c > .. > >> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h >> index 33a0ee3bcb2e..a89205c70ffa 100644 >> --- a/include/linux/syscalls.h >> +++ b/include/linux/syscalls.h >> @@ -71,6 +71,7 @@ struct clone_args; >> struct open_how; >> struct mount_attr; >> struct landlock_ruleset_attr; >> +struct lsm_ctx; >> enum landlock_rule_type; >> >> #include <linux/types.h> >> @@ -1058,6 +1059,8 @@ asmlinkage long sys_memfd_secret(unsigned int flags); >> asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len, >> unsigned long home_node, >> unsigned long flags); >> +asmlinkage long sys_lsm_get_self_attr(struct lsm_ctx *ctx, size_t *size, >> + int flags); >> >> /* >> * Architecture-specific system calls >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h >> index 61a91b7d946f..8674d8c6b326 100644 >> --- a/include/uapi/linux/lsm.h >> +++ b/include/uapi/linux/lsm.h >> @@ -9,6 +9,27 @@ >> #ifndef _UAPI_LINUX_LSM_H >> #define _UAPI_LINUX_LSM_H >> >> +#include <linux/types.h> >> +#include <linux/unistd.h> >> + >> +/** >> + * struct lsm_ctx - LSM context >> + * @id: the LSM id number, see LSM_ID_XXX > As mentioned above, it occurred to me that we might want want to pad > out the lsm_ctx struct to ensure that the "array" of lsm_ctx is nicely > aligned. I know some systems used to complain about unaligned > accesses, and even on those that don't complain tend to be faster when > access are aligned. We can either implicitly align the individual > lsm_ctx structs or we can add a total length field (in addition to the > @ctx_len field) so that the padding/alignment is explicit. > > Adding an explicit total length field could have some other advantages > in that it, in conjunction with the existing @flags field, would allow > an individual LSM to "extend" the lsm_ctx struct to provide additional > LSM specific information in the case where the single @ctx field was > not sufficient. Think of it as some additional future proofing in > addition to explicit padding. I'm not sure whether to call it future proofing or confusion assurance, and I certainly wouldn't encourage such use. On the other hand, I wouldn't let that get in the way of the performant aligned interface, so I'm good with the len field in addition to the ctx_len. >> + * @flags: context specifier and LSM specific flags > * @flags: LSM specific flags > > Only the individual LSM specified in @id should ever interpret @flags or @ctx. Sure. >> + * @ctx_len: the size of @ctx >> + * @ctx: the LSM context, a nul terminated string > * @ctx: the LSM context value As above, I would like to make this a string. I won't fight over it however. >> + * @ctx in a nul terminated string. >> + * (strlen(@ctx) < @ctx_len) is always true. >> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed. >> + */ > Let's rework the extra description too based on the comments above. > For the sake of clarity, here is what I'm currently thinking (comments > and feedback are encouraged): > > /** > * struct lsm_ctx - LSM context information > * @id: the LSM ID token, see LSM_ID_XXX > * @flags: LSM specific flags > * @len: length of the lsm_ctx struct + extra (?) + padding > * @ctx_len: the size of @ctx > * @ctx: the LSM context value > * > * The @len field MUST be equal to size of the lsm_ctx struct > * plus any additional padding and/or data placed after @ctx. > * > * In all cases @ctx_len MUST be equal to length of @ctx. If > * @ctx is a string value, it should be nul terminated with > * @ctx_len equal to `strlen(@ctx) + 1`. Binary @ctx values > * are supported. > * > * The @flags and @ctx fields SHOULD only be interpreted by the > * LSM specified by @id; they MUST be set to zero/0 when not used. > */ > struct lsm_ctx { > __u32 id; > __u64 flags; > __kernel_size_t len; > __kernel_size_t ctx_len; > __u8 ctx[]; > }; Yes, if there's a convincing argument for binary values. >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c >> new file mode 100644 >> index 000000000000..55e8bf61ac8a >> --- /dev/null >> +++ b/security/lsm_syscalls.c >> @@ -0,0 +1,182 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * System calls implementing the Linux Security Module API. >> + * >> + * Copyright (C) 2022 Casey Schaufler <casey@schaufler-ca.com> >> + * Copyright (C) 2022 Intel Corporation >> + */ >> + >> +#include <asm/current.h> >> +#include <linux/compiler_types.h> >> +#include <linux/err.h> >> +#include <linux/errno.h> >> +#include <linux/security.h> >> +#include <linux/stddef.h> >> +#include <linux/syscalls.h> >> +#include <linux/types.h> >> +#include <linux/lsm_hooks.h> >> +#include <uapi/linux/lsm.h> >> + >> +struct attrs_used_map { >> + char *name; >> + int attrs_used; > Based on the usage below it really seems like @attrs_used should just > be @attr, yes? That said, I'm not too bothered by it either way so if > you really love @attrs_used that's fine. Potato - Potatoe - Seeing the same field name in different structs at the same time gives me a headache, but I'm not going to quibble. >> +}; >> + >> +static const struct attrs_used_map lsm_attr_names[] = { > We can probably just call this "attr_map" right? I mean the "used" > portion is pretty inherent in the fact that we're defining a mapping > :) Sure. >> + { .name = "current", .attrs_used = LSM_ATTR_CURRENT, }, >> + { .name = "exec", .attrs_used = LSM_ATTR_EXEC, }, >> + { .name = "fscreate", .attrs_used = LSM_ATTR_FSCREATE, }, >> + { .name = "keycreate", .attrs_used = LSM_ATTR_KEYCREATE, }, >> + { .name = "prev", .attrs_used = LSM_ATTR_PREV, }, >> + { .name = "sockcreate", .attrs_used = LSM_ATTR_SOCKCREATE, }, >> +}; >> + >> +static int attr_used_index(u32 flags) > Since you can only return one index value at a time in this function > you can't really support multiple attribute bits set in the @flags > parameter so why not change the prototype to better match the required > usage, example: > > static int attr_index(u32 attr) > >> +{ >> + int i; >> + >> + if (flags == 0) >> + return -EINVAL; >> + >> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) >> + if ((lsm_attr_names[i].attrs_used & flags) == flags) >> + return i; > Given the above, why not simplify the above test to this: > > if (lsm_attr_name[i].attr == attr) > return i; That won't work in the case where an LSM supports more than one attribute. > > If we don't care about failing fast in the case of being passed 0 (why > would we?) we can define this function as follows: > > static int attr_index(u32 attr) > { > int i; > for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > if (lsm_attr_names[i].attr == attr) > return i; > return -EINVAL; > } > > If we wanted to streamline things even further we could define > attr_map a bit differently and drop the loop in attr_index(). Yes, it > does waste attr_map[0], but I don't think anyone is going to be too > upset about one wasted index if it scales better. > > static const struct attr_map[] = { > [LSM_ATTR_CURRENT] = { .name = "current", .attr = LSM_ATTR_CURRENT }, > [LSM_ATTR_EXEC] = { .name = "exec", .attr = LSM_ATTR_EXEC }, > ... > }; > > static int attr_index(u32 attr) > { > if (attr == 0 || attr >= ARRAY_SIZE(attr_map)) > return -EINVAL; > return attr; > } > > If you did this you could probably also convert attr_map from a struct > to a simple array of strings as the attribute value would be the > associated index. > >> +/** >> + * sys_lsm_get_self_attr - Return current task's security module attributes >> + * @ctx: the LSM contexts >> + * @size: size of @ctx, updated on return >> + * @flags: which attribute to return >> + * >> + * Returns the calling task's LSM contexts. On success this >> + * function returns the number of @ctx array elements. This value >> + * may be zero if there are no LSM contexts assigned. If @size is >> + * insufficient to contain the return data -E2BIG is returned and >> + * @size is set to the minimum required size. In all other cases >> + * a negative value indicating the error is returned. >> + */ >> +SYSCALL_DEFINE3(lsm_get_self_attr, >> + struct lsm_ctx __user *, ctx, >> + size_t __user *, size, >> + u32, flags) >> +{ >> + int i; >> + int rc = 0; >> + int len; >> + int attr; >> + int count = 0; >> + void *curr; >> + char *cp; >> + char *np; >> + char **interum_ctx; >> + size_t total_size = 0; >> + struct lsm_ctx *ip; >> + struct lsm_ctx *interum; >> + struct lsm_ctx *final = NULL; >> + >> + attr = attr_used_index(flags); >> + if (attr < 0) >> + return attr; >> + >> + interum = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * >> + sizeof(*interum), GFP_KERNEL); >> + if (interum == NULL) >> + return -ENOMEM; >> + ip = interum; >> + >> + interum_ctx = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * >> + sizeof(*interum_ctx), GFP_KERNEL); >> + if (interum_ctx == NULL) { >> + kfree(interum); >> + return -ENOMEM; >> + } >> + >> + for (i = 0; i < lsm_active_cnt; i++) { >> + if ((lsm_idlist[i]->attrs_used & >> + lsm_attr_names[attr].attrs_used) == 0) >> + continue; >> + >> + len = security_getprocattr(current, lsm_idlist[i]->id, >> + lsm_attr_names[attr].name, >> + &cp); >> + if (len <= 0) >> + continue; >> + >> + ip->id = lsm_idlist[i]->id; >> + ip->flags = lsm_attr_names[attr].attrs_used; >> + interum_ctx[count] = cp; >> + >> + /* >> + * A security module that returns a binary attribute >> + * will need to identify itself to prevent string >> + * processing. >> + * >> + * At least one security module adds a \n at the >> + * end of a context to make it look nicer. Change >> + * that to a \0 so that user space doesn't have to >> + * work around it. >> + * >> + * Security modules have been inconsistent about >> + * including the \0 terminator in the size. If it's >> + * not there make space for it. >> + * >> + * The length returned will reflect the length of >> + * the string provided by the security module, which >> + * may not match what getprocattr returned. >> + */ >> + np = strnchr(cp, len, '\n'); >> + if (np != NULL) >> + *np = '\0'; >> + ip->ctx_len = strnlen(cp, len) + 1; >> + total_size += sizeof(*interum) + ip->ctx_len; >> + ip++; >> + count++; >> + } >> + >> + if (count == 0) >> + goto free_out; >> + >> + final = kzalloc(total_size, GFP_KERNEL); >> + if (final == NULL) { >> + rc = -ENOMEM; >> + goto free_out; >> + } >> + >> + curr = final; >> + ip = interum; >> + for (i = 0; i < count; i++) { >> + memcpy(curr, ip, sizeof(*interum)); >> + curr += sizeof(*interum); >> + if (ip->ctx_len > 1) >> + memcpy(curr, interum_ctx[i], ip->ctx_len - 1); >> + curr += ip->ctx_len; >> + ip++; >> + } >> + >> + if (get_user(len, size)) { >> + rc = -EFAULT; >> + goto free_out; >> + } >> + if (total_size > len) { >> + rc = -ERANGE; >> + if (put_user(total_size, size) != 0) >> + rc = -EFAULT; >> + goto free_out; >> + } >> + if (copy_to_user(ctx, final, total_size) != 0 || >> + put_user(total_size, size) != 0) >> + rc = -EFAULT; >> + else >> + rc = count; >> + >> +free_out: >> + for (i = 0; i < count; i++) >> + kfree(interum_ctx[i]); >> + kfree(interum_ctx); >> + kfree(interum); >> + kfree(final); >> + return rc; >> +} > Hmm. That's all kinda painful isn't it? It's not so bad as all that. Well, maybe. > I think trying to reuse > security_getprocattr() is doing more harm than good with all the > awkward handling necessary to ensure consistent output. While it's > nice to be able to reuse existing interfaces, one of the main > motivations behind the LSM syscall effort is to create a cleaner > interface that was designed from the beginning to support multiple > LSMs and provide a level of extensibility that we do not currently > have with the procfs interface. Hacking together all our old crap to > make this happen seems very wrong to me. > > With that in mind I would like to propose we introduce a new LSM hook > to populate a lsm_ctx struct based on a LSM_ATTR_XXX value: > > int security_sys_getselfattr(u64 attr, struct lsm_ctx __user *ctx, > size_t *len); > > The individual LSMs would be responsible for fully populating their > lsm_ctx struct specified by @ctx (note the __user tagging) and would > return 0 on success or negative values on failure. The maximum size > of the @ctx buffer would be passed in via @len and the used size would > be returned in @len; in the case of an too-small @ctx, -E2BIG would be > returned and the necessary size would be returned in @len (just as > discussed for the syscall itself). This way the LSM layer syscall > function would not need to worry about properly terminating the > lsm_ctx::ctx field, setting any LSM specific flags, etc. Passing the > __user pointer directly not only greatly simplifies the LSM layer, it > also has the potential to reduce the number of allocations/copies. That's going to be a lot of duplicate code to add to each LSM. Yes, we can do that, but I don't see it as any cleaner. > Taking this approach should shrink the LSM layer syscall function to > simply needing to validate the passed @flags before looping through > the LSMs calling security_sys_getselfattr(). The lsm_ctx pointer > would need to be incremented appropriately for each call, and a total > length/size count would need to be maintained in case the buffer is > too small, but those should be relatively minor things. I think this pushed the complexity downward. If we only had one LSM it would be a wash. With each LSM having to provide this we're multiplying the complexity rather than reducing it. But again, I'll listen to reason. > > -- > paul-moore.com
On Mon, Jan 9, 2023, at 19:07, Casey Schaufler wrote: > +/** > + * struct lsm_ctx - LSM context > + * @id: the LSM id number, see LSM_ID_XXX > + * @flags: context specifier and LSM specific flags > + * @ctx_len: the size of @ctx > + * @ctx: the LSM context, a nul terminated string > + * > + * @ctx in a nul terminated string. > + * (strlen(@ctx) < @ctx_len) is always true. > + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed. > + */ > +struct lsm_ctx { > + __u32 id; > + __u64 flags; > + __kernel_size_t ctx_len; > + __u8 ctx[]; > +}; I think this should be changed to be the same layout on all architectures regardless of __u64 alignment and sizeof(__kernel_size_t) differences, to avoid the need for compat syscalls and explicit clearing of the internal padding. Maybe just use __u64 fields for all three integers? Arnd
On Wed, Jan 11, 2023 at 8:37 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 1/11/2023 1:07 PM, Paul Moore wrote: > > On Mon, Jan 9, 2023 at 1:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > >> Create a system call lsm_get_self_attr() to provide the security > >> module maintained attributes of the current process. Historically > >> these attributes have been exposed to user space via entries in > >> procfs under /proc/self/attr. > >> > >> Attributes are provided as a collection of lsm_ctx structures > >> which are placed into a user supplied buffer. Each structure > >> identifys the size of the attribute, and the attribute value. > > ^^^ > > identifies > > Pluralses are hard in the vicinity of 'y's. ;) > > >> The format of the attribute value is defined by the security > >> module, but will always be \0 terminated. The ctx_len value > >> will always be strlen(ctx)+1. > > I don't want to limit ourselves to only sending string values as > > attributes as who knows what we might need to do in the future, and > > the struct was originally designed to support both strings and binary > > data. I would suggest changing the sentences above to something like > > this: > > Part of creating a sane and sensible API is setting rational limitations. > While a "context" has always allowed for a binary value it has always > been a user friendly, nul terminated string. The one case where someone > proposed otherwise was my "hideous" format for compound contexts, and we > know how well that was received. If we're serious about cleaning up our > API let's quit bending over to support something no one is using and that > we'd prefer they didn't. I agree that I'm in no rush to move away from a simple string format for context values, but as we are at a unique point in time where we get to define a new API I think it is important to ensure that it has enough flexibility to endure whatever changes might come along in the next 10~20 years. I mean ~20 years ago we only had one LSM and the concept of containers/namespaces in the kernel was just a fringe concept (if at all!). I think it's also important to remember that we still review code around here, and just because the struct has the necessary provisions to *support* a binary context, it doesn't mean we are going to allow it to be used that way without a lot of discussion first. Any move to a binary format would have to be done in a way that doesn't break existing applications which would likely mean either a secondary LSM ID for those LSMs which provided a new format, a new LSM specific flag, and in the most extreme case a LSM specific override tacked to the end of the struct after the ctx field (possible since we now have the total length field). > That's my preference. Is there anyone who wants a binary interface, or any > really good reason to provide one? I'm not going to stand fast on a strings > only interface, but it would make it significantly cleaner if we had one. It's not about needing or even wanting it right now, it's about keeping it as a possible option for some point in the future when we might need it. > > The format of the attribute value is defined by the individual LSM, > > with the attribute itself stored in @ctx and the length of the > > attribute stored in @ctx_len. Both strings and arbitrary binary > > attributes are supported, but strings should be NULL terminated and > > @ctx_len should be equal to `strlen(@ctx) + 1`. > > > >> --------------------------- > >> | __u32 id | > >> --------------------------- > >> | __u64 flags | > >> --------------------------- > >> | __kernel_size_t ctx_len | > >> --------------------------- > >> | __u8 ctx[ctx_len] | > >> --------------------------- > >> | __u32 id | > >> --------------------------- > >> | __u64 flags | > >> --------------------------- > >> | __kernel_size_t ctx_len | > >> --------------------------- > >> | __u8 ctx[ctx_len] | > >> --------------------------- > > Don't repeat the structure layout in memory twice here, it's > > confusing. I also think it would be easier to read, and arguably more > > useful, to simply copy the struct definition into the description > > instead of the ASCII art column. > > OK on both. > > > Although, this has got me wondering if we should think about aligning > > the lsm_ctx structs when we are populating them in the kernel; more on > > this below ... > > As you say below, we'll need a total_len for this, but OK. > > > >> --- > >> Documentation/userspace-api/lsm.rst | 9 ++ > >> include/linux/syscalls.h | 3 + > >> include/uapi/linux/lsm.h | 21 ++++ > >> kernel/sys_ni.c | 3 + > >> security/Makefile | 1 + > >> security/lsm_syscalls.c | 182 ++++++++++++++++++++++++++++ > >> 6 files changed, 219 insertions(+) > >> create mode 100644 security/lsm_syscalls.c > > .. > > > >> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h > >> index 33a0ee3bcb2e..a89205c70ffa 100644 > >> --- a/include/linux/syscalls.h > >> +++ b/include/linux/syscalls.h > >> @@ -71,6 +71,7 @@ struct clone_args; > >> struct open_how; > >> struct mount_attr; > >> struct landlock_ruleset_attr; > >> +struct lsm_ctx; > >> enum landlock_rule_type; > >> > >> #include <linux/types.h> > >> @@ -1058,6 +1059,8 @@ asmlinkage long sys_memfd_secret(unsigned int flags); > >> asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len, > >> unsigned long home_node, > >> unsigned long flags); > >> +asmlinkage long sys_lsm_get_self_attr(struct lsm_ctx *ctx, size_t *size, > >> + int flags); > >> > >> /* > >> * Architecture-specific system calls > >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h > >> index 61a91b7d946f..8674d8c6b326 100644 > >> --- a/include/uapi/linux/lsm.h > >> +++ b/include/uapi/linux/lsm.h > >> @@ -9,6 +9,27 @@ > >> #ifndef _UAPI_LINUX_LSM_H > >> #define _UAPI_LINUX_LSM_H > >> > >> +#include <linux/types.h> > >> +#include <linux/unistd.h> > >> + > >> +/** > >> + * struct lsm_ctx - LSM context > >> + * @id: the LSM id number, see LSM_ID_XXX > > As mentioned above, it occurred to me that we might want want to pad > > out the lsm_ctx struct to ensure that the "array" of lsm_ctx is nicely > > aligned. I know some systems used to complain about unaligned > > accesses, and even on those that don't complain tend to be faster when > > access are aligned. We can either implicitly align the individual > > lsm_ctx structs or we can add a total length field (in addition to the > > @ctx_len field) so that the padding/alignment is explicit. > > > > Adding an explicit total length field could have some other advantages > > in that it, in conjunction with the existing @flags field, would allow > > an individual LSM to "extend" the lsm_ctx struct to provide additional > > LSM specific information in the case where the single @ctx field was > > not sufficient. Think of it as some additional future proofing in > > addition to explicit padding. > > I'm not sure whether to call it future proofing or confusion assurance, > and I certainly wouldn't encourage such use. On the other hand, I wouldn't > let that get in the way of the performant aligned interface, so I'm good > with the len field in addition to the ctx_len. Whatever you want to call it is fine by me. The more I think about it, the more I think this will be important to have. > >> + * @flags: context specifier and LSM specific flags > > * @flags: LSM specific flags > > > > Only the individual LSM specified in @id should ever interpret @flags or @ctx. > > Sure. > > >> + * @ctx_len: the size of @ctx > >> + * @ctx: the LSM context, a nul terminated string > > * @ctx: the LSM context value > > As above, I would like to make this a string. I won't fight over it however. > > >> + * @ctx in a nul terminated string. > >> + * (strlen(@ctx) < @ctx_len) is always true. > >> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed. > >> + */ > > Let's rework the extra description too based on the comments above. > > For the sake of clarity, here is what I'm currently thinking (comments > > and feedback are encouraged): > > > > /** > > * struct lsm_ctx - LSM context information > > * @id: the LSM ID token, see LSM_ID_XXX > > * @flags: LSM specific flags > > * @len: length of the lsm_ctx struct + extra (?) + padding > > * @ctx_len: the size of @ctx > > * @ctx: the LSM context value > > * > > * The @len field MUST be equal to size of the lsm_ctx struct > > * plus any additional padding and/or data placed after @ctx. > > * > > * In all cases @ctx_len MUST be equal to length of @ctx. If > > * @ctx is a string value, it should be nul terminated with > > * @ctx_len equal to `strlen(@ctx) + 1`. Binary @ctx values > > * are supported. > > * > > * The @flags and @ctx fields SHOULD only be interpreted by the > > * LSM specified by @id; they MUST be set to zero/0 when not used. > > */ > > struct lsm_ctx { > > __u32 id; > > __u64 flags; > > __kernel_size_t len; > > __kernel_size_t ctx_len; > > __u8 ctx[]; > > }; > > Yes, if there's a convincing argument for binary values. > > >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c > >> new file mode 100644 > >> index 000000000000..55e8bf61ac8a > >> --- /dev/null > >> +++ b/security/lsm_syscalls.c > >> @@ -0,0 +1,182 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +/* > >> + * System calls implementing the Linux Security Module API. > >> + * > >> + * Copyright (C) 2022 Casey Schaufler <casey@schaufler-ca.com> > >> + * Copyright (C) 2022 Intel Corporation > >> + */ > >> + > >> +#include <asm/current.h> > >> +#include <linux/compiler_types.h> > >> +#include <linux/err.h> > >> +#include <linux/errno.h> > >> +#include <linux/security.h> > >> +#include <linux/stddef.h> > >> +#include <linux/syscalls.h> > >> +#include <linux/types.h> > >> +#include <linux/lsm_hooks.h> > >> +#include <uapi/linux/lsm.h> > >> + > >> +struct attrs_used_map { > >> + char *name; > >> + int attrs_used; > > Based on the usage below it really seems like @attrs_used should just > > be @attr, yes? That said, I'm not too bothered by it either way so if > > you really love @attrs_used that's fine. > > Potato - Potatoe - Seeing the same field name in different structs at > the same time gives me a headache, but I'm not going to quibble. > > > >> +}; > >> + > >> +static const struct attrs_used_map lsm_attr_names[] = { > > We can probably just call this "attr_map" right? I mean the "used" > > portion is pretty inherent in the fact that we're defining a mapping > > :) > > Sure. > > > >> + { .name = "current", .attrs_used = LSM_ATTR_CURRENT, }, > >> + { .name = "exec", .attrs_used = LSM_ATTR_EXEC, }, > >> + { .name = "fscreate", .attrs_used = LSM_ATTR_FSCREATE, }, > >> + { .name = "keycreate", .attrs_used = LSM_ATTR_KEYCREATE, }, > >> + { .name = "prev", .attrs_used = LSM_ATTR_PREV, }, > >> + { .name = "sockcreate", .attrs_used = LSM_ATTR_SOCKCREATE, }, > >> +}; > >> + > >> +static int attr_used_index(u32 flags) > > Since you can only return one index value at a time in this function > > you can't really support multiple attribute bits set in the @flags > > parameter so why not change the prototype to better match the required > > usage, example: > > > > static int attr_index(u32 attr) > > > >> +{ > >> + int i; > >> + > >> + if (flags == 0) > >> + return -EINVAL; > >> + > >> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > >> + if ((lsm_attr_names[i].attrs_used & flags) == flags) > >> + return i; > > Given the above, why not simplify the above test to this: > > > > if (lsm_attr_name[i].attr == attr) > > return i; > > That won't work in the case where an LSM supports more than one attribute. > > > > > If we don't care about failing fast in the case of being passed 0 (why > > would we?) we can define this function as follows: > > > > static int attr_index(u32 attr) > > { > > int i; > > for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) > > if (lsm_attr_names[i].attr == attr) > > return i; > > return -EINVAL; > > } > > > > If we wanted to streamline things even further we could define > > attr_map a bit differently and drop the loop in attr_index(). Yes, it > > does waste attr_map[0], but I don't think anyone is going to be too > > upset about one wasted index if it scales better. > > > > static const struct attr_map[] = { > > [LSM_ATTR_CURRENT] = { .name = "current", .attr = LSM_ATTR_CURRENT }, > > [LSM_ATTR_EXEC] = { .name = "exec", .attr = LSM_ATTR_EXEC }, > > ... > > }; > > > > static int attr_index(u32 attr) > > { > > if (attr == 0 || attr >= ARRAY_SIZE(attr_map)) > > return -EINVAL; > > return attr; > > } > > > > If you did this you could probably also convert attr_map from a struct > > to a simple array of strings as the attribute value would be the > > associated index. > > > >> +/** > >> + * sys_lsm_get_self_attr - Return current task's security module attributes > >> + * @ctx: the LSM contexts > >> + * @size: size of @ctx, updated on return > >> + * @flags: which attribute to return > >> + * > >> + * Returns the calling task's LSM contexts. On success this > >> + * function returns the number of @ctx array elements. This value > >> + * may be zero if there are no LSM contexts assigned. If @size is > >> + * insufficient to contain the return data -E2BIG is returned and > >> + * @size is set to the minimum required size. In all other cases > >> + * a negative value indicating the error is returned. > >> + */ > >> +SYSCALL_DEFINE3(lsm_get_self_attr, > >> + struct lsm_ctx __user *, ctx, > >> + size_t __user *, size, > >> + u32, flags) > >> +{ > >> + int i; > >> + int rc = 0; > >> + int len; > >> + int attr; > >> + int count = 0; > >> + void *curr; > >> + char *cp; > >> + char *np; > >> + char **interum_ctx; > >> + size_t total_size = 0; > >> + struct lsm_ctx *ip; > >> + struct lsm_ctx *interum; > >> + struct lsm_ctx *final = NULL; > >> + > >> + attr = attr_used_index(flags); > >> + if (attr < 0) > >> + return attr; > >> + > >> + interum = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * > >> + sizeof(*interum), GFP_KERNEL); > >> + if (interum == NULL) > >> + return -ENOMEM; > >> + ip = interum; > >> + > >> + interum_ctx = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * > >> + sizeof(*interum_ctx), GFP_KERNEL); > >> + if (interum_ctx == NULL) { > >> + kfree(interum); > >> + return -ENOMEM; > >> + } > >> + > >> + for (i = 0; i < lsm_active_cnt; i++) { > >> + if ((lsm_idlist[i]->attrs_used & > >> + lsm_attr_names[attr].attrs_used) == 0) > >> + continue; > >> + > >> + len = security_getprocattr(current, lsm_idlist[i]->id, > >> + lsm_attr_names[attr].name, > >> + &cp); > >> + if (len <= 0) > >> + continue; > >> + > >> + ip->id = lsm_idlist[i]->id; > >> + ip->flags = lsm_attr_names[attr].attrs_used; > >> + interum_ctx[count] = cp; > >> + > >> + /* > >> + * A security module that returns a binary attribute > >> + * will need to identify itself to prevent string > >> + * processing. > >> + * > >> + * At least one security module adds a \n at the > >> + * end of a context to make it look nicer. Change > >> + * that to a \0 so that user space doesn't have to > >> + * work around it. > >> + * > >> + * Security modules have been inconsistent about > >> + * including the \0 terminator in the size. If it's > >> + * not there make space for it. > >> + * > >> + * The length returned will reflect the length of > >> + * the string provided by the security module, which > >> + * may not match what getprocattr returned. > >> + */ > >> + np = strnchr(cp, len, '\n'); > >> + if (np != NULL) > >> + *np = '\0'; > >> + ip->ctx_len = strnlen(cp, len) + 1; > >> + total_size += sizeof(*interum) + ip->ctx_len; > >> + ip++; > >> + count++; > >> + } > >> + > >> + if (count == 0) > >> + goto free_out; > >> + > >> + final = kzalloc(total_size, GFP_KERNEL); > >> + if (final == NULL) { > >> + rc = -ENOMEM; > >> + goto free_out; > >> + } > >> + > >> + curr = final; > >> + ip = interum; > >> + for (i = 0; i < count; i++) { > >> + memcpy(curr, ip, sizeof(*interum)); > >> + curr += sizeof(*interum); > >> + if (ip->ctx_len > 1) > >> + memcpy(curr, interum_ctx[i], ip->ctx_len - 1); > >> + curr += ip->ctx_len; > >> + ip++; > >> + } > >> + > >> + if (get_user(len, size)) { > >> + rc = -EFAULT; > >> + goto free_out; > >> + } > >> + if (total_size > len) { > >> + rc = -ERANGE; > >> + if (put_user(total_size, size) != 0) > >> + rc = -EFAULT; > >> + goto free_out; > >> + } > >> + if (copy_to_user(ctx, final, total_size) != 0 || > >> + put_user(total_size, size) != 0) > >> + rc = -EFAULT; > >> + else > >> + rc = count; > >> + > >> +free_out: > >> + for (i = 0; i < count; i++) > >> + kfree(interum_ctx[i]); > >> + kfree(interum_ctx); > >> + kfree(interum); > >> + kfree(final); > >> + return rc; > >> +} > > Hmm. That's all kinda painful isn't it? > > It's not so bad as all that. Well, maybe. > > > I think trying to reuse > > security_getprocattr() is doing more harm than good with all the > > awkward handling necessary to ensure consistent output. While it's > > nice to be able to reuse existing interfaces, one of the main > > motivations behind the LSM syscall effort is to create a cleaner > > interface that was designed from the beginning to support multiple > > LSMs and provide a level of extensibility that we do not currently > > have with the procfs interface. Hacking together all our old crap to > > make this happen seems very wrong to me. > > > > With that in mind I would like to propose we introduce a new LSM hook > > to populate a lsm_ctx struct based on a LSM_ATTR_XXX value: > > > > int security_sys_getselfattr(u64 attr, struct lsm_ctx __user *ctx, > > size_t *len); > > > > The individual LSMs would be responsible for fully populating their > > lsm_ctx struct specified by @ctx (note the __user tagging) and would > > return 0 on success or negative values on failure. The maximum size > > of the @ctx buffer would be passed in via @len and the used size would > > be returned in @len; in the case of an too-small @ctx, -E2BIG would be > > returned and the necessary size would be returned in @len (just as > > discussed for the syscall itself). This way the LSM layer syscall > > function would not need to worry about properly terminating the > > lsm_ctx::ctx field, setting any LSM specific flags, etc. Passing the > > __user pointer directly not only greatly simplifies the LSM layer, it > > also has the potential to reduce the number of allocations/copies. > > That's going to be a lot of duplicate code to add to each LSM. Yes, > we can do that, but I don't see it as any cleaner. It puts the individual LSMs in charge of setting their own lsm_ctx struct which I think will be increasingly important as time goes on and things evolve. I'd rather make sure the infrastructure is doing the right thing now so we can avoid having to have this exact same discussion each time an individual LSM wants to do something a little different with their lsm_ctx struct :) If you need something more concrete and immediate to justify this work, think about how a LSM could set a bit in the lsm_ctx::flags field using the current approach of reusing the procfs hook. > > Taking this approach should shrink the LSM layer syscall function to > > simply needing to validate the passed @flags before looping through > > the LSMs calling security_sys_getselfattr(). The lsm_ctx pointer > > would need to be incremented appropriately for each call, and a total > > length/size count would need to be maintained in case the buffer is > > too small, but those should be relatively minor things. > > I think this pushed the complexity downward. If we only had one LSM > it would be a wash. With each LSM having to provide this we're multiplying > the complexity rather than reducing it. > > But again, I'll listen to reason. I think we are all better off if the LSM layer is as thin as possible. I believe we've seen the number of LSMs grow and the variety of security models expand largely in part of this approach; while a thin LSM layer may force the LSMs to do some extra work, it allows varied approaches that might not be possible if the LSM layer enforced more of a specific world-view.
On Thu, Jan 12, 2023 at 9:40 AM Arnd Bergmann <arnd@arndb.de> wrote: > On Mon, Jan 9, 2023, at 19:07, Casey Schaufler wrote: > > +/** > > + * struct lsm_ctx - LSM context > > + * @id: the LSM id number, see LSM_ID_XXX > > + * @flags: context specifier and LSM specific flags > > + * @ctx_len: the size of @ctx > > + * @ctx: the LSM context, a nul terminated string > > + * > > + * @ctx in a nul terminated string. > > + * (strlen(@ctx) < @ctx_len) is always true. > > + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed. > > + */ > > +struct lsm_ctx { > > + __u32 id; > > + __u64 flags; > > + __kernel_size_t ctx_len; > > + __u8 ctx[]; > > +}; > > I think this should be changed to be the same layout on > all architectures regardless of __u64 alignment and > sizeof(__kernel_size_t) differences, to avoid the need > for compat syscalls and explicit clearing of the > internal padding. > > Maybe just use __u64 fields for all three integers? I have no problem with that ... the ctx[] field is variable length anyway so keeping it as a __u8 should be fine.
On Mon, Jan 09, 2023 at 10:07:13AM -0800, Casey Schaufler wrote: > +/** > + * sys_lsm_get_self_attr - Return current task's security module attributes > + * @ctx: the LSM contexts > + * @size: size of @ctx, updated on return > + * @flags: which attribute to return > + * > + * Returns the calling task's LSM contexts. On success this > + * function returns the number of @ctx array elements. This value > + * may be zero if there are no LSM contexts assigned. If @size is > + * insufficient to contain the return data -E2BIG is returned and Technicality: You say -E2BIG, which is -7, but in fact belog you return -ERANGE, which is -34. > + * @size is set to the minimum required size. In all other cases > + * a negative value indicating the error is returned. > + */ > +SYSCALL_DEFINE3(lsm_get_self_attr, > + struct lsm_ctx __user *, ctx, > + size_t __user *, size, > + u32, flags) > +{ > + int i; > + int rc = 0; > + int len; > + int attr; > + int count = 0; > + void *curr; > + char *cp; > + char *np; > + char **interum_ctx; > + size_t total_size = 0; > + struct lsm_ctx *ip; > + struct lsm_ctx *interum; > + struct lsm_ctx *final = NULL; > + > + attr = attr_used_index(flags); > + if (attr < 0) > + return attr; > + > + interum = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * > + sizeof(*interum), GFP_KERNEL); > + if (interum == NULL) > + return -ENOMEM; > + ip = interum; > + > + interum_ctx = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * > + sizeof(*interum_ctx), GFP_KERNEL); > + if (interum_ctx == NULL) { > + kfree(interum); > + return -ENOMEM; > + } > + > + for (i = 0; i < lsm_active_cnt; i++) { > + if ((lsm_idlist[i]->attrs_used & > + lsm_attr_names[attr].attrs_used) == 0) > + continue; > + > + len = security_getprocattr(current, lsm_idlist[i]->id, > + lsm_attr_names[attr].name, > + &cp); > + if (len <= 0) > + continue; > + > + ip->id = lsm_idlist[i]->id; > + ip->flags = lsm_attr_names[attr].attrs_used; > + interum_ctx[count] = cp; > + > + /* > + * A security module that returns a binary attribute > + * will need to identify itself to prevent string > + * processing. > + * > + * At least one security module adds a \n at the > + * end of a context to make it look nicer. Change > + * that to a \0 so that user space doesn't have to > + * work around it. > + * > + * Security modules have been inconsistent about > + * including the \0 terminator in the size. If it's > + * not there make space for it. > + * > + * The length returned will reflect the length of > + * the string provided by the security module, which > + * may not match what getprocattr returned. > + */ > + np = strnchr(cp, len, '\n'); > + if (np != NULL) > + *np = '\0'; > + ip->ctx_len = strnlen(cp, len) + 1; > + total_size += sizeof(*interum) + ip->ctx_len; > + ip++; > + count++; > + } > + > + if (count == 0) > + goto free_out; > + > + final = kzalloc(total_size, GFP_KERNEL); > + if (final == NULL) { > + rc = -ENOMEM; > + goto free_out; > + } > + > + curr = final; > + ip = interum; > + for (i = 0; i < count; i++) { > + memcpy(curr, ip, sizeof(*interum)); > + curr += sizeof(*interum); > + if (ip->ctx_len > 1) > + memcpy(curr, interum_ctx[i], ip->ctx_len - 1); > + curr += ip->ctx_len; > + ip++; > + } > + > + if (get_user(len, size)) { > + rc = -EFAULT; > + goto free_out; > + } > + if (total_size > len) { > + rc = -ERANGE; > + if (put_user(total_size, size) != 0) > + rc = -EFAULT; > + goto free_out; > + } > + if (copy_to_user(ctx, final, total_size) != 0 || > + put_user(total_size, size) != 0) > + rc = -EFAULT; > + else > + rc = count; > + > +free_out: > + for (i = 0; i < count; i++) > + kfree(interum_ctx[i]); > + kfree(interum_ctx); > + kfree(interum); > + kfree(final); > + return rc; > +} > -- > 2.39.0 >
On 12/01/2023 22:39, Paul Moore wrote: > On Thu, Jan 12, 2023 at 9:40 AM Arnd Bergmann <arnd@arndb.de> wrote: >> On Mon, Jan 9, 2023, at 19:07, Casey Schaufler wrote: >>> +/** >>> + * struct lsm_ctx - LSM context >>> + * @id: the LSM id number, see LSM_ID_XXX >>> + * @flags: context specifier and LSM specific flags >>> + * @ctx_len: the size of @ctx >>> + * @ctx: the LSM context, a nul terminated string >>> + * >>> + * @ctx in a nul terminated string. >>> + * (strlen(@ctx) < @ctx_len) is always true. >>> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed. >>> + */ >>> +struct lsm_ctx { >>> + __u32 id; There is a hole here for 64-bit architectures. >>> + __u64 flags; >>> + __kernel_size_t ctx_len; This is an architecture-related size, which makes the struct size different according to architectures. We should avoid that. >>> + __u8 ctx[]; >>> +}; I suggest packing this struct. >> >> I think this should be changed to be the same layout on >> all architectures regardless of __u64 alignment and >> sizeof(__kernel_size_t) differences, to avoid the need >> for compat syscalls and explicit clearing of the >> internal padding. >> >> Maybe just use __u64 fields for all three integers? > > I have no problem with that ... the ctx[] field is variable length > anyway so keeping it as a __u8 should be fine. > For Landlock, we make sure the UAPI structs don't contain holes, are packed, and have the same size for all architectures. We can check that with pahole but for strong guarantee I suggest the same build check as for Landlock's build_check_abi(): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/landlock/syscalls.c#n68 We don't need to use 64-bit fields everywhere.
On 09/01/2023 19:07, Casey Schaufler wrote: > Create a system call lsm_get_self_attr() to provide the security > module maintained attributes of the current process. Historically > these attributes have been exposed to user space via entries in > procfs under /proc/self/attr. > > Attributes are provided as a collection of lsm_ctx structures > which are placed into a user supplied buffer. Each structure > identifys the size of the attribute, and the attribute value. > The format of the attribute value is defined by the security > module, but will always be \0 terminated. The ctx_len value > will always be strlen(ctx)+1. > > --------------------------- > | __u32 id | > --------------------------- > | __u64 flags | > --------------------------- > | __kernel_size_t ctx_len | > --------------------------- > | __u8 ctx[ctx_len] | > --------------------------- > | __u32 id | > --------------------------- > | __u64 flags | > --------------------------- > | __kernel_size_t ctx_len | > --------------------------- > | __u8 ctx[ctx_len] | > --------------------------- > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > Documentation/userspace-api/lsm.rst | 9 ++ > include/linux/syscalls.h | 3 + > include/uapi/linux/lsm.h | 21 ++++ > kernel/sys_ni.c | 3 + > security/Makefile | 1 + > security/lsm_syscalls.c | 182 ++++++++++++++++++++++++++++ > 6 files changed, 219 insertions(+) > create mode 100644 security/lsm_syscalls.c For new files (e.g. lsm_syscalls.c), it would be nice to auto-format them with clang-format. It helps maintenance by keeping a consistent style across commits, which should also help backports, and it avoids nitpicking on style issues.
On 2/14/2023 9:41 AM, Mickaël Salaün wrote: > > On 09/01/2023 19:07, Casey Schaufler wrote: >> Create a system call lsm_get_self_attr() to provide the security >> module maintained attributes of the current process. Historically >> these attributes have been exposed to user space via entries in >> procfs under /proc/self/attr. >> >> Attributes are provided as a collection of lsm_ctx structures >> which are placed into a user supplied buffer. Each structure >> identifys the size of the attribute, and the attribute value. >> The format of the attribute value is defined by the security >> module, but will always be \0 terminated. The ctx_len value >> will always be strlen(ctx)+1. >> >> --------------------------- >> | __u32 id | >> --------------------------- >> | __u64 flags | >> --------------------------- >> | __kernel_size_t ctx_len | >> --------------------------- >> | __u8 ctx[ctx_len] | >> --------------------------- >> | __u32 id | >> --------------------------- >> | __u64 flags | >> --------------------------- >> | __kernel_size_t ctx_len | >> --------------------------- >> | __u8 ctx[ctx_len] | >> --------------------------- >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> Documentation/userspace-api/lsm.rst | 9 ++ >> include/linux/syscalls.h | 3 + >> include/uapi/linux/lsm.h | 21 ++++ >> kernel/sys_ni.c | 3 + >> security/Makefile | 1 + >> security/lsm_syscalls.c | 182 ++++++++++++++++++++++++++++ >> 6 files changed, 219 insertions(+) >> create mode 100644 security/lsm_syscalls.c > > For new files (e.g. lsm_syscalls.c), it would be nice to auto-format > them with clang-format. It helps maintenance by keeping a consistent > style across commits, which should also help backports, and it avoids > nitpicking on style issues. Good idea.
diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst index 6ddf5506110b..98a0c191b499 100644 --- a/Documentation/userspace-api/lsm.rst +++ b/Documentation/userspace-api/lsm.rst @@ -48,6 +48,15 @@ creating socket objects. The proc filesystem provides this value in ``/proc/self/attr/sockcreate``. This is supported by the SELinux security module. +Kernel interface +================ + +Get the security attributes of the current process +-------------------------------------------------- + +.. kernel-doc:: security/lsm_syscalls.c + :identifiers: sys_lsm_get_self_attr + Additional documentation ======================== diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index 33a0ee3bcb2e..a89205c70ffa 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -71,6 +71,7 @@ struct clone_args; struct open_how; struct mount_attr; struct landlock_ruleset_attr; +struct lsm_ctx; enum landlock_rule_type; #include <linux/types.h> @@ -1058,6 +1059,8 @@ asmlinkage long sys_memfd_secret(unsigned int flags); asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len, unsigned long home_node, unsigned long flags); +asmlinkage long sys_lsm_get_self_attr(struct lsm_ctx *ctx, size_t *size, + int flags); /* * Architecture-specific system calls diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h index 61a91b7d946f..8674d8c6b326 100644 --- a/include/uapi/linux/lsm.h +++ b/include/uapi/linux/lsm.h @@ -9,6 +9,27 @@ #ifndef _UAPI_LINUX_LSM_H #define _UAPI_LINUX_LSM_H +#include <linux/types.h> +#include <linux/unistd.h> + +/** + * struct lsm_ctx - LSM context + * @id: the LSM id number, see LSM_ID_XXX + * @flags: context specifier and LSM specific flags + * @ctx_len: the size of @ctx + * @ctx: the LSM context, a nul terminated string + * + * @ctx in a nul terminated string. + * (strlen(@ctx) < @ctx_len) is always true. + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed. + */ +struct lsm_ctx { + __u32 id; + __u64 flags; + __kernel_size_t ctx_len; + __u8 ctx[]; +}; + /* * ID values to identify security modules. * A system may use more than one security module. diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c index 860b2dcf3ac4..7b2513d5605d 100644 --- a/kernel/sys_ni.c +++ b/kernel/sys_ni.c @@ -262,6 +262,9 @@ COND_SYSCALL_COMPAT(recvmsg); /* mm/nommu.c, also with MMU */ COND_SYSCALL(mremap); +/* security/lsm_syscalls.c */ +COND_SYSCALL(lsm_get_self_attr); + /* security/keys/keyctl.c */ COND_SYSCALL(add_key); COND_SYSCALL(request_key); diff --git a/security/Makefile b/security/Makefile index 18121f8f85cd..59f238490665 100644 --- a/security/Makefile +++ b/security/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_KEYS) += keys/ # always enable default capabilities obj-y += commoncap.o +obj-$(CONFIG_SECURITY) += lsm_syscalls.o obj-$(CONFIG_MMU) += min_addr.o # Object file lists diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c new file mode 100644 index 000000000000..55e8bf61ac8a --- /dev/null +++ b/security/lsm_syscalls.c @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * System calls implementing the Linux Security Module API. + * + * Copyright (C) 2022 Casey Schaufler <casey@schaufler-ca.com> + * Copyright (C) 2022 Intel Corporation + */ + +#include <asm/current.h> +#include <linux/compiler_types.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/security.h> +#include <linux/stddef.h> +#include <linux/syscalls.h> +#include <linux/types.h> +#include <linux/lsm_hooks.h> +#include <uapi/linux/lsm.h> + +struct attrs_used_map { + char *name; + int attrs_used; +}; + +static const struct attrs_used_map lsm_attr_names[] = { + { .name = "current", .attrs_used = LSM_ATTR_CURRENT, }, + { .name = "exec", .attrs_used = LSM_ATTR_EXEC, }, + { .name = "fscreate", .attrs_used = LSM_ATTR_FSCREATE, }, + { .name = "keycreate", .attrs_used = LSM_ATTR_KEYCREATE, }, + { .name = "prev", .attrs_used = LSM_ATTR_PREV, }, + { .name = "sockcreate", .attrs_used = LSM_ATTR_SOCKCREATE, }, +}; + +static int attr_used_index(u32 flags) +{ + int i; + + if (flags == 0) + return -EINVAL; + + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++) + if ((lsm_attr_names[i].attrs_used & flags) == flags) + return i; + + return -EINVAL; +} + +/** + * sys_lsm_get_self_attr - Return current task's security module attributes + * @ctx: the LSM contexts + * @size: size of @ctx, updated on return + * @flags: which attribute to return + * + * Returns the calling task's LSM contexts. On success this + * function returns the number of @ctx array elements. This value + * may be zero if there are no LSM contexts assigned. If @size is + * insufficient to contain the return data -E2BIG is returned and + * @size is set to the minimum required size. In all other cases + * a negative value indicating the error is returned. + */ +SYSCALL_DEFINE3(lsm_get_self_attr, + struct lsm_ctx __user *, ctx, + size_t __user *, size, + u32, flags) +{ + int i; + int rc = 0; + int len; + int attr; + int count = 0; + void *curr; + char *cp; + char *np; + char **interum_ctx; + size_t total_size = 0; + struct lsm_ctx *ip; + struct lsm_ctx *interum; + struct lsm_ctx *final = NULL; + + attr = attr_used_index(flags); + if (attr < 0) + return attr; + + interum = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * + sizeof(*interum), GFP_KERNEL); + if (interum == NULL) + return -ENOMEM; + ip = interum; + + interum_ctx = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_active_cnt * + sizeof(*interum_ctx), GFP_KERNEL); + if (interum_ctx == NULL) { + kfree(interum); + return -ENOMEM; + } + + for (i = 0; i < lsm_active_cnt; i++) { + if ((lsm_idlist[i]->attrs_used & + lsm_attr_names[attr].attrs_used) == 0) + continue; + + len = security_getprocattr(current, lsm_idlist[i]->id, + lsm_attr_names[attr].name, + &cp); + if (len <= 0) + continue; + + ip->id = lsm_idlist[i]->id; + ip->flags = lsm_attr_names[attr].attrs_used; + interum_ctx[count] = cp; + + /* + * A security module that returns a binary attribute + * will need to identify itself to prevent string + * processing. + * + * At least one security module adds a \n at the + * end of a context to make it look nicer. Change + * that to a \0 so that user space doesn't have to + * work around it. + * + * Security modules have been inconsistent about + * including the \0 terminator in the size. If it's + * not there make space for it. + * + * The length returned will reflect the length of + * the string provided by the security module, which + * may not match what getprocattr returned. + */ + np = strnchr(cp, len, '\n'); + if (np != NULL) + *np = '\0'; + ip->ctx_len = strnlen(cp, len) + 1; + total_size += sizeof(*interum) + ip->ctx_len; + ip++; + count++; + } + + if (count == 0) + goto free_out; + + final = kzalloc(total_size, GFP_KERNEL); + if (final == NULL) { + rc = -ENOMEM; + goto free_out; + } + + curr = final; + ip = interum; + for (i = 0; i < count; i++) { + memcpy(curr, ip, sizeof(*interum)); + curr += sizeof(*interum); + if (ip->ctx_len > 1) + memcpy(curr, interum_ctx[i], ip->ctx_len - 1); + curr += ip->ctx_len; + ip++; + } + + if (get_user(len, size)) { + rc = -EFAULT; + goto free_out; + } + if (total_size > len) { + rc = -ERANGE; + if (put_user(total_size, size) != 0) + rc = -EFAULT; + goto free_out; + } + if (copy_to_user(ctx, final, total_size) != 0 || + put_user(total_size, size) != 0) + rc = -EFAULT; + else + rc = count; + +free_out: + for (i = 0; i < count; i++) + kfree(interum_ctx[i]); + kfree(interum_ctx); + kfree(interum); + kfree(final); + return rc; +}