* Re: [PATCH v20 16/23] LSM: Use lsmcontext in security_inode_getsecctx
From: Paul Moore @ 2020-09-06 2:55 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <20200826145247.10029-17-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:19 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the security_inode_getsecctx() interface to fill
> a lsmcontext structure instead of data and length pointers.
> This provides the information about which LSM created the
> context so that security_release_secctx() can use the
> correct hook.
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> fs/nfsd/nfs4xdr.c | 23 +++++++++--------------
> include/linux/security.h | 5 +++--
> security/security.c | 13 +++++++++++--
> 3 files changed, 23 insertions(+), 18 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 14/23] LSM: Ensure the correct LSM context releaser
From: Paul Moore @ 2020-09-06 2:45 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-integrity, netdev
In-Reply-To: <20200826145247.10029-15-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:16 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Add a new lsmcontext data structure to hold all the information
> about a "security context", including the string, its size and
> which LSM allocated the string. The allocation information is
> necessary because LSMs have different policies regarding the
> lifecycle of these strings. SELinux allocates and destroys
> them on each use, whereas Smack provides a pointer to an entry
> in a list that never goes away.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-integrity@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
> drivers/android/binder.c | 10 ++++---
> fs/ceph/xattr.c | 6 ++++-
> fs/nfs/nfs4proc.c | 8 ++++--
> fs/nfsd/nfs4xdr.c | 7 +++--
> include/linux/security.h | 35 +++++++++++++++++++++++--
> include/net/scm.h | 5 +++-
> kernel/audit.c | 14 +++++++---
> kernel/auditsc.c | 12 ++++++---
> net/ipv4/ip_sockglue.c | 4 ++-
> net/netfilter/nf_conntrack_netlink.c | 4 ++-
> net/netfilter/nf_conntrack_standalone.c | 4 ++-
> net/netfilter/nfnetlink_queue.c | 13 ++++++---
> net/netlabel/netlabel_unlabeled.c | 19 +++++++++++---
> net/netlabel/netlabel_user.c | 4 ++-
> security/security.c | 11 ++++----
> 15 files changed, 121 insertions(+), 35 deletions(-)
One small comment below, but otherwise ...
Acked-by: Paul Moore <paul@paul-moore.com>
> +/**
> + * lsmcontext_init - initialize an lsmcontext structure.
> + * @cp: Pointer to the context to initialize
> + * @context: Initial context, or NULL
> + * @size: Size of context, or 0
> + * @slot: Which LSM provided the context
> + *
> + * Fill in the lsmcontext from the provided information.
> + * This is a scaffolding function that will be removed when
> + * lsmcontext integration is complete.
> + */
> +static inline void lsmcontext_init(struct lsmcontext *cp, char *context,
> + u32 size, int slot)
> +{
> + cp->slot = slot;
> + cp->context = context;
> + cp->len = size;
> +}
Here is another case where some of the intermediate code, and perhaps
some of the final code, can probably be simplified if
lsmcontext_init() returns the lsmcontext pointer.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 12/23] IMA: Change internal interfaces to use lsmblobs
From: Paul Moore @ 2020-09-06 2:28 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-integrity
In-Reply-To: <20200826145247.10029-13-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:14 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> The IMA interfaces ima_get_action() and ima_match_policy()
> call LSM functions that use lsmblobs. Change the IMA functions
> to pass the lsmblob to be compatible with the LSM functions.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-integrity@vger.kernel.org
> ---
> security/integrity/ima/ima.h | 11 +++++----
> security/integrity/ima/ima_api.c | 10 ++++----
> security/integrity/ima/ima_appraise.c | 6 ++---
> security/integrity/ima/ima_main.c | 35 +++++++++++----------------
> security/integrity/ima/ima_policy.c | 14 +++++------
> 5 files changed, 34 insertions(+), 42 deletions(-)
...
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index a86b35dad4fa..b057c758b430 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -519,7 +519,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> case LSM_SUBJ_USER:
> case LSM_SUBJ_ROLE:
> case LSM_SUBJ_TYPE:
> - lsmblob_init(&lsmdata, secid);
> rc = ima_filter_rule_match(&lsmdata, rule->lsm[i].type,
> Audit_equal,
> rule->lsm[i].rules);
I'm jumping across patches in this patchset so I may have missed
something, but I think the ima_filter_rule_match() call should be
using the passed "blob" pointer and not the local "lsmdata" right? If
this is correct, I think this patch can also remove the local
"lsmdata" as well.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: John Johansen @ 2020-09-05 19:05 UTC (permalink / raw)
To: Casey Schaufler, Paul Moore
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, penguin-kernel, Stephen Smalley
In-Reply-To: <585600d7-70fb-0982-1e6b-ffd7b7c33e32@schaufler-ca.com>
On 9/5/20 11:13 AM, Casey Schaufler wrote:
> On 9/5/2020 6:25 AM, Paul Moore wrote:
>> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>> ...
>>
>>>> I understand the concerns you mention, they are all valid as far as
>>>> I'm concerned, but I think we are going to get burned by this code as
>>>> it currently stands.
>>> Yes, I can see that. We're getting burned by the non-extensibility
>>> of secids. It will take someone smarter than me to figure out how to
>>> fit N secids into 32bits without danger of either failure or memory
>>> allocation.
>> Sooo what are the next steps here? It sounds like there is some
>> agreement that the currently proposed unix_skb_params approach is a
>> problem, but it also sounds like you just want to merge it anyway?
>
> There are real problems with all the approaches. This is by far the
> least invasive of the lot. If this is acceptable for now I will commit
> to including the dynamic allocation version in the full stacking
> (e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
> to take even longer than it already has. Sigh.
>
>
>> I was sorta hoping for something a bit better.
>
> I will be looking at alternatives. I am very much open to suggestions.
> I'm not even 100% convinced that Stephen's objections to my separate
> allocation strategy outweigh its advantages. If you have an opinion on
> that, I'd love to hear it.
>
fwiw I prefer the separate allocation strategy, but as you have already
said it trading off one set of problems for another. I would rather see
this move forward and one set of trade offs isn't significantly worse
than the other to me so, either wfm.
^ permalink raw reply
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: Casey Schaufler @ 2020-09-05 18:13 UTC (permalink / raw)
To: Paul Moore
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, Casey Schaufler
In-Reply-To: <CAHC9VhSu4qqKWsutm3=GF_pihUKpwjAtc9gAhfjGsGtKfz-Azw@mail.gmail.com>
On 9/5/2020 6:25 AM, Paul Moore wrote:
> On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 9/4/2020 2:53 PM, Paul Moore wrote:
>>> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 9/4/2020 1:08 PM, Paul Moore wrote:
> ...
>
>>> I understand the concerns you mention, they are all valid as far as
>>> I'm concerned, but I think we are going to get burned by this code as
>>> it currently stands.
>> Yes, I can see that. We're getting burned by the non-extensibility
>> of secids. It will take someone smarter than me to figure out how to
>> fit N secids into 32bits without danger of either failure or memory
>> allocation.
> Sooo what are the next steps here? It sounds like there is some
> agreement that the currently proposed unix_skb_params approach is a
> problem, but it also sounds like you just want to merge it anyway?
There are real problems with all the approaches. This is by far the
least invasive of the lot. If this is acceptable for now I will commit
to including the dynamic allocation version in the full stacking
(e.g. Smack + SELinux) stage. If it isn't, well, this stage is going
to take even longer than it already has. Sigh.
> I was sorta hoping for something a bit better.
I will be looking at alternatives. I am very much open to suggestions.
I'm not even 100% convinced that Stephen's objections to my separate
allocation strategy outweigh its advantages. If you have an opinion on
that, I'd love to hear it.
Thank you
^ permalink raw reply
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: Paul Moore @ 2020-09-05 13:25 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <ef6a049a-c6b9-370b-c521-4594aa73e403@schaufler-ca.com>
On Fri, Sep 4, 2020 at 7:58 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 9/4/2020 2:53 PM, Paul Moore wrote:
> > On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 9/4/2020 1:08 PM, Paul Moore wrote:
...
> > I understand the concerns you mention, they are all valid as far as
> > I'm concerned, but I think we are going to get burned by this code as
> > it currently stands.
>
> Yes, I can see that. We're getting burned by the non-extensibility
> of secids. It will take someone smarter than me to figure out how to
> fit N secids into 32bits without danger of either failure or memory
> allocation.
Sooo what are the next steps here? It sounds like there is some
agreement that the currently proposed unix_skb_params approach is a
problem, but it also sounds like you just want to merge it anyway?
I was sorta hoping for something a bit better.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 10/23] LSM: Use lsmblob in security_inode_getsecid
From: Paul Moore @ 2020-09-05 13:20 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-integrity
In-Reply-To: <20200826145247.10029-11-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:12 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the security_inode_getsecid() interface to fill in a
> lsmblob structure instead of a u32 secid. This allows for its
> callers to gather data from all registered LSMs. Data is provided
> for IMA and audit.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> cc: linux-integrity@vger.kernel.org
> ---
> include/linux/security.h | 7 ++++---
> kernel/auditsc.c | 6 +++++-
> security/integrity/ima/ima_policy.c | 4 +---
> security/security.c | 11 +++++++++--
> 4 files changed, 19 insertions(+), 9 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 09/23] LSM: Use lsmblob in security_task_getsecid
From: Paul Moore @ 2020-09-05 13:18 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-integrity
In-Reply-To: <20200826145247.10029-10-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:11 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the security_task_getsecid() interface to fill in
> a lsmblob structure instead of a u32 secid in support of
> LSM stacking. Audit interfaces will need to collect all
> possible secids for possible reporting.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: linux-integrity@vger.kernel.org
> ---
> drivers/android/binder.c | 12 +------
> include/linux/security.h | 7 ++--
> kernel/audit.c | 16 ++++-----
> kernel/auditfilter.c | 4 +--
> kernel/auditsc.c | 25 +++++++-------
> net/netlabel/netlabel_unlabeled.c | 5 ++-
> net/netlabel/netlabel_user.h | 6 +++-
> security/integrity/ima/ima_appraise.c | 10 +++---
> security/integrity/ima/ima_main.c | 49 +++++++++++++++------------
> security/security.c | 12 +++++--
> 10 files changed, 76 insertions(+), 70 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 08/23] LSM: Use lsmblob in security_ipc_getsecid
From: Paul Moore @ 2020-09-05 13:12 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <20200826145247.10029-9-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:10 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> There may be more than one LSM that provides IPC data
> for auditing. Change security_ipc_getsecid() to fill in
> a lsmblob structure instead of the u32 secid. The
> audit data structure containing the secid will be updated
> later, so there is a bit of scaffolding here.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/security.h | 7 ++++---
> kernel/auditsc.c | 7 ++++++-
> security/security.c | 12 +++++++++---
> 3 files changed, 19 insertions(+), 7 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH V2 2/3] integrity: Move import of MokListRT certs to a separate routine
From: Lenny Szubowicz @ 2020-09-05 1:31 UTC (permalink / raw)
To: linux-kernel, linux-efi, platform-driver-x86,
linux-security-module, andy.shevchenko, ardb, jmorris, serge,
keescook, zohar, bp, pjones, dhowells, prarit
In-Reply-To: <20200905013107.10457-1-lszubowi@redhat.com>
Move the loading of certs from the UEFI MokListRT into a separate
routine to facilitate additional MokList functionality.
There is no visible functional change as a result of this patch.
Although the UEFI dbx certs are now loaded before the MokList certs,
they are loaded onto different key rings. So the order of the keys
on their respective key rings is the same.
Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
---
security/integrity/platform_certs/load_uefi.c | 63 +++++++++++++------
1 file changed, 44 insertions(+), 19 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 253fb9a7fc98..c1c622b4dc78 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -66,6 +66,43 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
}
/*
+ * load_moklist_certs() - Load MokList certs
+ *
+ * Load the certs contained in the UEFI MokListRT database into the
+ * platform trusted keyring.
+ *
+ * Return: Status
+ */
+static int __init load_moklist_certs(void)
+{
+ efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
+ void *mok;
+ unsigned long moksize;
+ efi_status_t status;
+ int rc;
+
+ /* Get MokListRT. It might not exist, so it isn't an error
+ * if we can't get it.
+ */
+ mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
+ if (mok) {
+ rc = parse_efi_signature_list("UEFI:MokListRT",
+ mok, moksize, get_handler_for_db);
+ kfree(mok);
+ if (rc)
+ pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
+ return rc;
+ }
+ if (status == EFI_NOT_FOUND)
+ pr_debug("MokListRT variable wasn't found\n");
+ else
+ pr_info("Couldn't get UEFI MokListRT\n");
+ return 0;
+}
+
+/*
+ * load_uefi_certs() - Load certs from UEFI sources
+ *
* Load the certs contained in the UEFI databases into the platform trusted
* keyring and the UEFI blacklisted X.509 cert SHA256 hashes into the blacklist
* keyring.
@@ -73,17 +110,16 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
static int __init load_uefi_certs(void)
{
efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
- efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
- void *db = NULL, *dbx = NULL, *mok = NULL;
- unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+ void *db = NULL, *dbx = NULL;
+ unsigned long dbsize = 0, dbxsize = 0;
efi_status_t status;
int rc = 0;
if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE))
return false;
- /* Get db, MokListRT, and dbx. They might not exist, so it isn't
- * an error if we can't get them.
+ /* Get db and dbx. They might not exist, so it isn't an error
+ * if we can't get them.
*/
if (!uefi_check_ignore_db()) {
db = get_cert_list(L"db", &secure_var, &dbsize, &status);
@@ -102,20 +138,6 @@ static int __init load_uefi_certs(void)
}
}
- mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
- if (!mok) {
- if (status == EFI_NOT_FOUND)
- pr_debug("MokListRT variable wasn't found\n");
- else
- pr_info("Couldn't get UEFI MokListRT\n");
- } else {
- rc = parse_efi_signature_list("UEFI:MokListRT",
- mok, moksize, get_handler_for_db);
- if (rc)
- pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
- kfree(mok);
- }
-
dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
if (!dbx) {
if (status == EFI_NOT_FOUND)
@@ -131,6 +153,9 @@ static int __init load_uefi_certs(void)
kfree(dbx);
}
+ /* Load the MokListRT certs */
+ rc = load_moklist_certs();
+
return rc;
}
late_initcall(load_uefi_certs);
--
2.27.0
^ permalink raw reply related
* [PATCH V2 1/3] efi: Support for MOK variable config table
From: Lenny Szubowicz @ 2020-09-05 1:31 UTC (permalink / raw)
To: linux-kernel, linux-efi, platform-driver-x86,
linux-security-module, andy.shevchenko, ardb, jmorris, serge,
keescook, zohar, bp, pjones, dhowells, prarit
In-Reply-To: <20200905013107.10457-1-lszubowi@redhat.com>
Because of system-specific EFI firmware limitations, EFI volatile
variables may not be capable of holding the required contents of
the Machine Owner Key (MOK) certificate store when the certificate
list grows above some size. Therefore, an EFI boot loader may pass
the MOK certs via a EFI configuration table created specifically for
this purpose to avoid this firmware limitation.
An EFI configuration table is a much more primitive mechanism
compared to EFI variables and is well suited for one-way passage
of static information from a pre-OS environment to the kernel.
This patch adds initial kernel support to recognize, parse,
and validate the EFI MOK configuration table, where named
entries contain the same data that would otherwise be provided
in similarly named EFI variables.
Additionally, this patch creates a sysfs binary file for each
EFI MOK configuration table entry found. These files are read-only
to root and are provided for use by user space utilities such as
mokutil.
A subsequent patch will load MOK certs into the trusted platform
key ring using this infrastructure.
Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
---
arch/x86/kernel/setup.c | 1 +
arch/x86/platform/efi/efi.c | 3 +
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/arm-init.c | 1 +
drivers/firmware/efi/efi.c | 6 +
drivers/firmware/efi/mokvar-table.c | 360 ++++++++++++++++++++++++++++
include/linux/efi.h | 34 +++
7 files changed, 406 insertions(+)
create mode 100644 drivers/firmware/efi/mokvar-table.c
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 3511736fbc74..d41be0df72f8 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1077,6 +1077,7 @@ void __init setup_arch(char **cmdline_p)
efi_fake_memmap();
efi_find_mirror();
efi_esrt_init();
+ efi_mokvar_table_init();
/*
* The EFI specification says that boot service code won't be
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index d37ebe6e70d7..8a26e705cb06 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -90,6 +90,9 @@ static const unsigned long * const efi_tables[] = {
&efi.tpm_log,
&efi.tpm_final_log,
&efi_rng_seed,
+#ifdef CONFIG_LOAD_UEFI_KEYS
+ &efi.mokvar_table,
+#endif
};
u64 efi_setup; /* efi setup_data physical address */
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 7a216984552b..03964e2d27c5 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_EFI_DEV_PATH_PARSER) += dev-path-parser.o
obj-$(CONFIG_APPLE_PROPERTIES) += apple-properties.o
obj-$(CONFIG_EFI_RCI2_TABLE) += rci2-table.o
obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE) += embedded-firmware.o
+obj-$(CONFIG_LOAD_UEFI_KEYS) += mokvar-table.o
fake_map-y += fake_mem.o
fake_map-$(CONFIG_X86) += x86_fake_mem.o
diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 71c445d20258..f55a92ff12c0 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -236,6 +236,7 @@ void __init efi_init(void)
reserve_regions();
efi_esrt_init();
+ efi_mokvar_table_init();
memblock_reserve(data.phys_map & PAGE_MASK,
PAGE_ALIGN(data.size + (data.phys_map & ~PAGE_MASK)));
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3aa07c3b5136..3d4daf215e19 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -43,6 +43,9 @@ struct efi __read_mostly efi = {
.esrt = EFI_INVALID_TABLE_ADDR,
.tpm_log = EFI_INVALID_TABLE_ADDR,
.tpm_final_log = EFI_INVALID_TABLE_ADDR,
+#ifdef CONFIG_LOAD_UEFI_KEYS
+ .mokvar_table = EFI_INVALID_TABLE_ADDR,
+#endif
};
EXPORT_SYMBOL(efi);
@@ -518,6 +521,9 @@ static const efi_config_table_type_t common_tables[] __initconst = {
{EFI_RT_PROPERTIES_TABLE_GUID, &rt_prop, "RTPROP" },
#ifdef CONFIG_EFI_RCI2_TABLE
{DELLEMC_EFI_RCI2_TABLE_GUID, &rci2_table_phys },
+#endif
+#ifdef CONFIG_LOAD_UEFI_KEYS
+ {LINUX_EFI_MOK_VARIABLE_TABLE_GUID, &efi.mokvar_table, "MOKvar" },
#endif
{},
};
diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c
new file mode 100644
index 000000000000..f12f1710f5d9
--- /dev/null
+++ b/drivers/firmware/efi/mokvar-table.c
@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * mokvar-table.c
+ *
+ * Copyright (c) 2020 Red Hat
+ * Author: Lenny Szubowicz <lszubowi@redhat.com>
+ *
+ * This module contains the kernel support for the Linux EFI Machine
+ * Owner Key (MOK) variable configuration table, which is identified by
+ * the LINUX_EFI_MOK_VARIABLE_TABLE_GUID.
+ *
+ * This EFI configuration table provides a more robust alternative to
+ * EFI volatile variables by which an EFI boot loader can pass the
+ * contents of the Machine Owner Key (MOK) certificate stores to the
+ * kernel during boot. If both the EFI MOK config table and corresponding
+ * EFI MOK variables are present, the table should be considered as
+ * more authoritative.
+ *
+ * This module includes code that validates and maps the EFI MOK table,
+ * if it's presence was detected very early in boot.
+ *
+ * Kernel interface routines are provided to walk through all the
+ * entries in the MOK config table or to search for a specific named
+ * entry.
+ *
+ * The contents of the individual named MOK config table entries are
+ * made available to user space via read-only sysfs binary files under:
+ *
+ * /sys/firmware/efi/mok-variables/
+ *
+ */
+#define pr_fmt(fmt) "mokvar: " fmt
+
+#include <linux/capability.h>
+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+
+/*
+ * The LINUX_EFI_MOK_VARIABLE_TABLE_GUID config table is a packed
+ * sequence of struct efi_mokvar_table_entry, one for each named
+ * MOK variable. The sequence is terminated by an entry with a
+ * completely NULL name and 0 data size.
+ *
+ * efi_mokvar_table_size is set to the computed size of the
+ * MOK config table by efi_mokvar_table_init(). This will be
+ * non-zero if and only if the table if present and has been
+ * validated by efi_mokvar_table_init().
+ */
+static size_t efi_mokvar_table_size;
+
+/*
+ * efi_mokvar_table_va is the kernel virtual address at which the
+ * EFI MOK config table has been mapped by efi_mokvar_sysfs_init().
+ */
+static struct efi_mokvar_table_entry *efi_mokvar_table_va;
+
+/*
+ * Each /sys/firmware/efi/mok-variables/ sysfs file is represented by
+ * an instance of struct efi_mokvar_sysfs_attr on efi_mokvar_sysfs_list.
+ * bin_attr.private points to the associated EFI MOK config table entry.
+ *
+ * This list is created during boot and then remains unchanged.
+ * So no sychronization is currently required to walk the list.
+ */
+struct efi_mokvar_sysfs_attr {
+ struct bin_attribute bin_attr;
+ struct list_head node;
+};
+
+static LIST_HEAD(efi_mokvar_sysfs_list);
+static struct kobject *mokvar_kobj;
+
+/*
+ * efi_mokvar_table_init() - Early boot validation of EFI MOK config table
+ *
+ * If present, validate and compute the size of the EFI MOK variable
+ * configuration table. This table may be provided by an EFI boot loader
+ * as an alternative to ordinary EFI variables, due to platform-dependent
+ * limitations. The memory occupied by this table is marked as reserved.
+ *
+ * This routine must be called before efi_free_boot_services() in order
+ * to guarantee that it can mark the table as reserved.
+ *
+ * Implicit inputs:
+ * efi.mokvar_table: Physical address of EFI MOK variable config table
+ * or special value that indicates no such table.
+ *
+ * Implicit outputs:
+ * efi_mokvar_table_size: Computed size of EFI MOK variable config table.
+ * The table is considered present and valid if this
+ * is non-zero.
+ */
+void __init efi_mokvar_table_init(void)
+{
+ efi_memory_desc_t md;
+ u64 end_pa;
+ void *va = NULL;
+ size_t cur_offset = 0;
+ size_t offset_limit;
+ size_t map_size = 0;
+ size_t map_size_needed = 0;
+ size_t size;
+ struct efi_mokvar_table_entry *mokvar_entry;
+ int err = -EINVAL;
+
+ if (!efi_enabled(EFI_MEMMAP))
+ return;
+
+ if (efi.mokvar_table == EFI_INVALID_TABLE_ADDR)
+ return;
+ /*
+ * The EFI MOK config table must fit within a single EFI memory
+ * descriptor range.
+ */
+ err = efi_mem_desc_lookup(efi.mokvar_table, &md);
+ if (err) {
+ pr_warn("EFI MOKvar config table is not within the EFI memory map\n");
+ return;
+ }
+ end_pa = efi_mem_desc_end(&md);
+ if (efi.mokvar_table >= end_pa) {
+ pr_err("EFI memory descriptor containing MOKvar config table is invalid\n");
+ return;
+ }
+ offset_limit = end_pa - efi.mokvar_table;
+ /*
+ * Validate the MOK config table. Since there is no table header
+ * from which we could get the total size of the MOK config table,
+ * we compute the total size as we validate each variably sized
+ * entry, remapping as necessary.
+ */
+ while (cur_offset + sizeof(*mokvar_entry) <= offset_limit) {
+ mokvar_entry = va + cur_offset;
+ map_size_needed = cur_offset + sizeof(*mokvar_entry);
+ if (map_size_needed > map_size) {
+ if (va)
+ early_memunmap(va, map_size);
+ /*
+ * Map a little more than the fixed size entry
+ * header, anticipating some data. It's safe to
+ * do so as long as we stay within current memory
+ * descriptor.
+ */
+ map_size = min(map_size_needed + 2*EFI_PAGE_SIZE,
+ offset_limit);
+ va = early_memremap(efi.mokvar_table, map_size);
+ if (!va) {
+ pr_err("Failed to map EFI MOKvar config table pa=0x%lx, size=%zu.\n",
+ efi.mokvar_table, map_size);
+ return;
+ }
+ mokvar_entry = va + cur_offset;
+ }
+
+ /* Check for last sentinel entry */
+ if (mokvar_entry->name[0] == '\0') {
+ if (mokvar_entry->data_size != 0)
+ break;
+ err = 0;
+ break;
+ }
+
+ /* Sanity check that the name is null terminated */
+ size = strnlen(mokvar_entry->name,
+ sizeof(mokvar_entry->name));
+ if (size >= sizeof(mokvar_entry->name))
+ break;
+
+ /* Advance to the next entry */
+ cur_offset = map_size_needed + mokvar_entry->data_size;
+ }
+
+ if (va)
+ early_memunmap(va, map_size);
+ if (err) {
+ pr_err("EFI MOKvar config table is not valid\n");
+ return;
+ }
+ efi_mem_reserve(efi.mokvar_table, map_size_needed);
+ efi_mokvar_table_size = map_size_needed;
+}
+
+/*
+ * efi_mokvar_entry_next() - Get next entry in the EFI MOK config table
+ *
+ * mokvar_entry: Pointer to current EFI MOK config table entry
+ * or null. Null indicates get first entry.
+ * Passed by reference. This is updated to the
+ * same value as the return value.
+ *
+ * Returns: Pointer to next EFI MOK config table entry
+ * or null, if there are no more entries.
+ * Same value is returned in the mokvar_entry
+ * parameter.
+ *
+ * This routine depends on the EFI MOK config table being entirely
+ * mapped with it's starting virtual address in efi_mokvar_table_va.
+ */
+struct efi_mokvar_table_entry *efi_mokvar_entry_next(
+ struct efi_mokvar_table_entry **mokvar_entry)
+{
+ struct efi_mokvar_table_entry *mokvar_cur;
+ struct efi_mokvar_table_entry *mokvar_next;
+ size_t size_cur;
+
+ mokvar_cur = *mokvar_entry;
+ *mokvar_entry = NULL;
+
+ if (efi_mokvar_table_va == NULL)
+ return NULL;
+
+ if (mokvar_cur == NULL) {
+ mokvar_next = efi_mokvar_table_va;
+ } else {
+ if (mokvar_cur->name[0] == '\0')
+ return NULL;
+ size_cur = sizeof(*mokvar_cur) + mokvar_cur->data_size;
+ mokvar_next = (void *)mokvar_cur + size_cur;
+ }
+
+ if (mokvar_next->name[0] == '\0')
+ return NULL;
+
+ *mokvar_entry = mokvar_next;
+ return mokvar_next;
+}
+
+/*
+ * efi_mokvar_entry_find() - Find EFI MOK config entry by name
+ *
+ * name: Name of the entry to look for.
+ *
+ * Returns: Pointer to EFI MOK config table entry if found;
+ * null otherwise.
+ *
+ * This routine depends on the EFI MOK config table being entirely
+ * mapped with it's starting virtual address in efi_mokvar_table_va.
+ */
+struct efi_mokvar_table_entry *efi_mokvar_entry_find(const char *name)
+{
+ struct efi_mokvar_table_entry *mokvar_entry = NULL;
+
+ while (efi_mokvar_entry_next(&mokvar_entry)) {
+ if (!strncmp(name, mokvar_entry->name,
+ sizeof(mokvar_entry->name)))
+ return mokvar_entry;
+ }
+ return NULL;
+}
+
+/*
+ * efi_mokvar_sysfs_read() - sysfs binary file read routine
+ *
+ * Returns: Count of bytes read.
+ *
+ * Copy EFI MOK config table entry data for this mokvar sysfs binary file
+ * to the supplied buffer, starting at the specified offset into mokvar table
+ * entry data, for the specified count bytes. The copy is limited by the
+ * amount of data in this mokvar config table entry.
+ */
+static ssize_t efi_mokvar_sysfs_read(struct file *file, struct kobject *kobj,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t off, size_t count)
+{
+ struct efi_mokvar_table_entry *mokvar_entry = bin_attr->private;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return 0;
+
+ if (off >= mokvar_entry->data_size)
+ return 0;
+ if (count > mokvar_entry->data_size - off)
+ count = mokvar_entry->data_size - off;
+
+ memcpy(buf, mokvar_entry->data + off, count);
+ return count;
+}
+
+/*
+ * efi_mokvar_sysfs_init() - Map EFI MOK config table and create sysfs
+ *
+ * Map the EFI MOK variable config table for run-time use by the kernel
+ * and create the sysfs entries in /sys/firmware/efi/mok-variables/
+ *
+ * This routine just returns if a valid EFI MOK variable config table
+ * was not found earlier during boot.
+ *
+ * This routine must be called during a "middle" initcall phase, i.e.
+ * after efi_mokvar_table_init() but before UEFI certs are loaded
+ * during late init.
+ *
+ * Implicit inputs:
+ * efi.mokvar_table: Physical address of EFI MOK variable config table
+ * or special value that indicates no such table.
+ *
+ * efi_mokvar_table_size: Computed size of EFI MOK variable config table.
+ * The table is considered present and valid if this
+ * is non-zero.
+ *
+ * Implicit outputs:
+ * efi_mokvar_table_va: Start virtual address of the EFI MOK config table.
+ */
+static int __init efi_mokvar_sysfs_init(void)
+{
+ void *config_va;
+ struct efi_mokvar_table_entry *mokvar_entry = NULL;
+ struct efi_mokvar_sysfs_attr *mokvar_sysfs = NULL;
+ int err = 0;
+
+ if (efi_mokvar_table_size == 0)
+ return -ENOENT;
+
+ config_va = memremap(efi.mokvar_table, efi_mokvar_table_size,
+ MEMREMAP_WB);
+ if (!config_va) {
+ pr_err("Failed to map EFI MOKvar config table\n");
+ return -ENOMEM;
+ }
+ efi_mokvar_table_va = config_va;
+
+ mokvar_kobj = kobject_create_and_add("mok-variables", efi_kobj);
+ if (!mokvar_kobj) {
+ pr_err("Failed to create EFI mok-variables sysfs entry\n");
+ return -ENOMEM;
+ }
+
+ while (efi_mokvar_entry_next(&mokvar_entry)) {
+ mokvar_sysfs = kzalloc(sizeof(*mokvar_sysfs), GFP_KERNEL);
+ if (!mokvar_sysfs) {
+ err = -ENOMEM;
+ break;
+ }
+
+ sysfs_bin_attr_init(&mokvar_sysfs->bin_attr);
+ mokvar_sysfs->bin_attr.private = mokvar_entry;
+ mokvar_sysfs->bin_attr.attr.name = mokvar_entry->name;
+ mokvar_sysfs->bin_attr.attr.mode = 0400;
+ mokvar_sysfs->bin_attr.size = mokvar_entry->data_size;
+ mokvar_sysfs->bin_attr.read = efi_mokvar_sysfs_read;
+
+ err = sysfs_create_bin_file(mokvar_kobj,
+ &mokvar_sysfs->bin_attr);
+ if (err)
+ break;
+
+ list_add_tail(&mokvar_sysfs->node, &efi_mokvar_sysfs_list);
+ }
+
+ if (err) {
+ pr_err("Failed to create some EFI mok-variables sysfs entries\n");
+ kfree(mokvar_sysfs);
+ }
+ return err;
+}
+device_initcall(efi_mokvar_sysfs_init);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 73db1ae04cef..4a2332f146eb 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -357,6 +357,7 @@ void efi_native_runtime_setup(void);
#define LINUX_EFI_TPM_FINAL_LOG_GUID EFI_GUID(0x1e2ed096, 0x30e2, 0x4254, 0xbd, 0x89, 0x86, 0x3b, 0xbe, 0xf8, 0x23, 0x25)
#define LINUX_EFI_MEMRESERVE_TABLE_GUID EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5, 0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
#define LINUX_EFI_INITRD_MEDIA_GUID EFI_GUID(0x5568e427, 0x68fc, 0x4f3d, 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68)
+#define LINUX_EFI_MOK_VARIABLE_TABLE_GUID EFI_GUID(0xc451ed2b, 0x9694, 0x45d3, 0xba, 0xba, 0xed, 0x9f, 0x89, 0x88, 0xa3, 0x89)
/* OEM GUIDs */
#define DELLEMC_EFI_RCI2_TABLE_GUID EFI_GUID(0x2d9f28a2, 0xa886, 0x456a, 0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
@@ -546,6 +547,7 @@ extern struct efi {
unsigned long esrt; /* ESRT table */
unsigned long tpm_log; /* TPM2 Event Log table */
unsigned long tpm_final_log; /* TPM2 Final Events Log table */
+ unsigned long mokvar_table; /* MOK variable config table */
efi_get_time_t *get_time;
efi_set_time_t *set_time;
@@ -1252,4 +1254,36 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 size);
char *efi_systab_show_arch(char *str);
+/*
+ * The LINUX_EFI_MOK_VARIABLE_TABLE_GUID config table can be provided
+ * to the kernel by an EFI boot loader. The table contains a packed
+ * sequence of these entries, one for each named MOK variable.
+ * The sequence is terminated by an entry with a completely NULL
+ * name and 0 data size.
+ */
+struct efi_mokvar_table_entry {
+ char name[256];
+ u64 data_size;
+ u8 data[];
+} __attribute((packed));
+
+#ifdef CONFIG_LOAD_UEFI_KEYS
+extern void __init efi_mokvar_table_init(void);
+extern struct efi_mokvar_table_entry *efi_mokvar_entry_next(
+ struct efi_mokvar_table_entry **mokvar_entry);
+extern struct efi_mokvar_table_entry *efi_mokvar_entry_find(const char *name);
+#else
+static inline void efi_mokvar_table_init(void) { }
+static inline struct efi_mokvar_table_entry *efi_mokvar_entry_next(
+ struct efi_mokvar_table_entry **mokvar_entry)
+{
+ return NULL;
+}
+static inline struct efi_mokvar_table_entry *efi_mokvar_entry_find(
+ const char *name)
+{
+ return NULL;
+}
+#endif
+
#endif /* _LINUX_EFI_H */
--
2.27.0
^ permalink raw reply related
* [PATCH V2 3/3] integrity: Load certs from the EFI MOK config table
From: Lenny Szubowicz @ 2020-09-05 1:31 UTC (permalink / raw)
To: linux-kernel, linux-efi, platform-driver-x86,
linux-security-module, andy.shevchenko, ardb, jmorris, serge,
keescook, zohar, bp, pjones, dhowells, prarit
In-Reply-To: <20200905013107.10457-1-lszubowi@redhat.com>
Because of system-specific EFI firmware limitations, EFI volatile
variables may not be capable of holding the required contents of
the Machine Owner Key (MOK) certificate store when the certificate
list grows above some size. Therefore, an EFI boot loader may pass
the MOK certs via a EFI configuration table created specifically for
this purpose to avoid this firmware limitation.
An EFI configuration table is a much more primitive mechanism
compared to EFI variables and is well suited for one-way passage
of static information from a pre-OS environment to the kernel.
This patch adds the support to load certs from the MokListRT
entry in the MOK variable configuration table, if it's present.
The pre-existing support to load certs from the MokListRT EFI
variable remains and is used if the EFI MOK configuration table
isn't present or can't be successfully used.
Signed-off-by: Lenny Szubowicz <lszubowi@redhat.com>
---
security/integrity/platform_certs/load_uefi.c | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index c1c622b4dc78..ee4b4c666854 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -71,16 +71,38 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
* Load the certs contained in the UEFI MokListRT database into the
* platform trusted keyring.
*
+ * This routine checks the EFI MOK config table first. If and only if
+ * that fails, this routine uses the MokListRT ordinary UEFI variable.
+ *
* Return: Status
*/
static int __init load_moklist_certs(void)
{
+ struct efi_mokvar_table_entry *mokvar_entry;
efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
void *mok;
unsigned long moksize;
efi_status_t status;
int rc;
+ /* First try to load certs from the EFI MOKvar config table.
+ * It's not an error if the MOKvar config table doesn't exist
+ * or the MokListRT entry is not found in it.
+ */
+ mokvar_entry = efi_mokvar_entry_find("MokListRT");
+ if (mokvar_entry) {
+ rc = parse_efi_signature_list("UEFI:MokListRT (MOKvar table)",
+ mokvar_entry->data,
+ mokvar_entry->data_size,
+ get_handler_for_db);
+ /* All done if that worked. */
+ if (!rc)
+ return rc;
+
+ pr_err("Couldn't parse MokListRT signatures from EFI MOKvar config table: %d\n",
+ rc);
+ }
+
/* Get MokListRT. It might not exist, so it isn't an error
* if we can't get it.
*/
--
2.27.0
^ permalink raw reply related
* [PATCH V2 0/3] integrity: Load certs from EFI MOK config table
From: Lenny Szubowicz @ 2020-09-05 1:31 UTC (permalink / raw)
To: linux-kernel, linux-efi, platform-driver-x86,
linux-security-module, andy.shevchenko, ardb, jmorris, serge,
keescook, zohar, bp, pjones, dhowells, prarit
Because of system-specific EFI firmware limitations, EFI volatile
variables may not be capable of holding the required contents of
the Machine Owner Key (MOK) certificate store when the certificate
list grows above some size. Therefore, an EFI boot loader may pass
the MOK certs via a EFI configuration table created specifically for
this purpose to avoid this firmware limitation.
An EFI configuration table is a simpler and more robust mechanism
compared to EFI variables and is well suited for one-way passage
of static information from a pre-OS environment to the kernel.
Entries in the MOK variable configuration table are named key/value
pairs. Therefore the shim boot loader can create a MokListRT named
entry in the MOK configuration table that contains exactly the same
data as the MokListRT UEFI variable does or would otherwise contain.
As such, the kernel can load certs from the data in the MokListRT
configuration table entry data in the same way that it loads certs
from the data returned by the EFI GetVariable() runtime call for the
MokListRT variable.
This patch set does not remove the support for loading certs from the
EFI MOK variables into the platform key ring. However, if both the EFI
MOK configuration table and corresponding EFI MOK variables are present,
the MOK table is used as the source of MOK certs.
The contents of the individual named MOK config table entries are
made available to user space as individual sysfs binary files,
which are read-only to root, under:
/sys/firmware/efi/mok-variables/
This enables an updated mokutil to provide support for:
mokutil --list-enrolled
such that it can provide accurate information regardless of whether
the MOK configuration table or MOK EFI variables were the source
for certs. Note that all modifications of MOK related state are still
initiated by mokutil via EFI variables.
V2: Incorporate feedback from V1
Patch 01: efi: Support for MOK variable config table
- Minor update to change log; no code changes
Patch 02: integrity: Move import of MokListRT certs to a separate routine
- Clean up code flow in code moved to load_moklist_certs()
- Remove some unnecessary initialization of variables
Patch 03: integrity: Load certs from the EFI MOK config table
- Update required due to changes in patch 02.
- Remove unnecessary init of mokvar_entry in load_moklist_certs()
V1:
https://lore.kernel.org/lkml/20200826034455.28707-1-lszubowi@redhat.com/
Lenny Szubowicz (3):
efi: Support for MOK variable config table
integrity: Move import of MokListRT certs to a separate routine
integrity: Load certs from the EFI MOK config table
arch/x86/kernel/setup.c | 1 +
arch/x86/platform/efi/efi.c | 3 +
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/arm-init.c | 1 +
drivers/firmware/efi/efi.c | 6 +
drivers/firmware/efi/mokvar-table.c | 360 ++++++++++++++++++
include/linux/efi.h | 34 ++
security/integrity/platform_certs/load_uefi.c | 85 ++++-
8 files changed, 472 insertions(+), 19 deletions(-)
create mode 100644 drivers/firmware/efi/mokvar-table.c
--
2.27.0
^ permalink raw reply
* Re: [PATCH 0/3] integrity: Load certs from EFI MOK config table
From: Lenny Szubowicz @ 2020-09-05 1:30 UTC (permalink / raw)
To: Mimi Zohar, linux-kernel, linux-efi, platform-driver-x86,
linux-security-module, ardb, jmorris, serge, keescook, bp, pjones,
dhowells, prarit
In-Reply-To: <6f63a0cf1349281ef2c407d95abedfba1f90345a.camel@linux.ibm.com>
On 8/26/20 7:55 AM, Mimi Zohar wrote:
> Hi Lenny,
>
> On Tue, 2020-08-25 at 23:44 -0400, Lenny Szubowicz wrote:
>> Because of system-specific EFI firmware limitations,
>> EFI volatile variables may not be capable of holding the
>> required contents of the Machine Owner Key (MOK) certificate
>> store. Therefore, an EFI boot loader may pass the MOK certs
>> via a EFI configuration table created specifically for this
>> purpose to avoid this firmware limitation.
>>
>> An EFI configuration table is a simpler and more robust mechanism
>> compared to EFI variables and is well suited for one-way passage
>> of static information from a pre-OS environment to the kernel.
>>
>> This patch set does not remove the support for loading certs
>> from the EFI MOK variables into the platform key ring.
>> However, if both the EFI MOK config table and corresponding
>> EFI MOK variables are present, the MOK table is used as the
>> source of MOK certs.
>>
>> The contents of the individual named MOK config table entries are
>> made available to user space via read-only sysfs binary files under:
>>
>> /sys/firmware/efi/mok-variables/
>
> Please include a security section in this cover letter with a
> comparison of the MoK variables and the EFI configuration table
> security (eg. same mechanism?). Has mokutil been updated? If so,
> please provide a link.
>
> Mimi
>
I've included some more information about the MOK config table
entries in the V2 cover letter.
[root@localhost ~]# ls -l /sys/firmware/efi/mok-variables
total 0
-r--------. 1 root root 0 Sep 4 21:10 MokIgnoreDB
-r--------. 1 root root 18184 Sep 4 21:10 MokListRT
-r--------. 1 root root 76 Sep 4 21:10 MokListXRT
-r--------. 1 root root 0 Sep 4 21:10 MokSBStateRT
The roughly 18KB of data in /sys/firmware/efi/mok-variables/MokListRT
is exactly the same data that is returned by a EFI GetVariable()
call for MokListRT. Of course, that's on a system where the EFI
firmware can handle a volatile variable with that much data.
Therefore, load_moklist_certs() can pass the mokvar_entry data directly
to parse_efi_signature_list() in the same way it does for the
efi.get_variable() data that it obtains via get_cert_list().
Unfortunately, there is no updated mokutil available yet that
uses the new sysfs entries.
Also relevant is availability of an updated shim, which builds
the EFI MOK variable configuration table.
Of course, both of these should show up as upstream pull requests
and also in Fedora rawhide at some point.
Thank you for your review.
-Lenny.
^ permalink raw reply
* Re: [PATCH 2/3] integrity: Move import of MokListRT certs to a separate routine
From: Lenny Szubowicz @ 2020-09-05 0:57 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linux Kernel Mailing List, linux-efi, Platform Driver,
linux-security-module, Ard Biesheuvel, James Morris,
Serge E. Hallyn, Kees Cook, Mimi Zohar, Borislav Petkov,
Peter Jones, David Howells, Prarit Bhargava
In-Reply-To: <CAHp75Vec0a3LC7dGY6wacQu0brc+Zjfowt6kGdcZ9sfMzoDR9g@mail.gmail.com>
On 9/2/20 3:55 AM, Andy Shevchenko wrote:
> On Wed, Aug 26, 2020 at 6:45 AM Lenny Szubowicz <lszubowi@redhat.com> wrote:
>>
>> Move the loading of certs from the UEFI MokListRT into a separate
>> routine to facilitate additional MokList functionality.
>>
>> There is no visible functional change as a result of this patch.
>> Although the UEFI dbx certs are now loaded before the MokList certs,
>> they are loaded onto different key rings. So the order of the keys
>> on their respective key rings is the same.
>
> ...
>
>> /*
>> + * load_moklist_certs() - Load MokList certs
>> + *
>> + * Returns: Summary error status
>> + *
>> + * Load the certs contained in the UEFI MokListRT database into the
>> + * platform trusted keyring.
>> + */
>
> Hmm... Is it intentionally kept out of kernel doc format?
Yes. Since this is a static local routine, I thought that it
shouldn't be included by kerneldoc. But I wanted to generally adhere
to the kernel doc conventions for a routine header. To that end,
in V2 I move the "Return:" section to come after the short description.
>
>> +static int __init load_moklist_certs(void)
>> +{
>> + efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>> + void *mok = NULL;
>> + unsigned long moksize = 0;
>> + efi_status_t status;
>> + int rc = 0;
>
> Redundant assignment (see below).
>
>> + /* Get MokListRT. It might not exist, so it isn't an error
>> + * if we can't get it.
>> + */
>> + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
>
>> + if (!mok) {
>
> Why not positive conditional? Sometimes ! is hard to notice.
>
>> + if (status == EFI_NOT_FOUND)
>> + pr_debug("MokListRT variable wasn't found\n");
>> + else
>> + pr_info("Couldn't get UEFI MokListRT\n");
>> + } else {
>> + rc = parse_efi_signature_list("UEFI:MokListRT",
>> + mok, moksize, get_handler_for_db);
>> + if (rc)
>> + pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
>> + kfree(mok);
>
> kfree(...)
> if (rc)
> ...
> return rc;
>
> And with positive conditional there will be no need to have redundant
> 'else' followed by additional level of indentation.
>
>> + }
>
>> + return rc;
>
> return 0;
>
>> +}
>
> P.S. Yes, I see that the above was in the original code, so, consider
> my comments as suggestions to improve the code.
>
I agree that your suggestions improve the code. I've incorporated this
into V2.
-Lenny.
^ permalink raw reply
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: Casey Schaufler @ 2020-09-04 23:58 UTC (permalink / raw)
To: Paul Moore
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, Casey Schaufler
In-Reply-To: <CAHC9VhSf8RWUnRPYLR6LLzbn-cvNg8J0wnZGwTOAe=dOqkvd0g@mail.gmail.com>
On 9/4/2020 2:53 PM, Paul Moore wrote:
> On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 9/4/2020 1:08 PM, Paul Moore wrote:
>>> On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> Change the data used in UDS SO_PEERSEC processing from a
>>>> secid to a more general struct lsmblob. Update the
>>>> security_socket_getpeersec_dgram() interface to use the
>>>> lsmblob. There is a small amount of scaffolding code
>>>> that will come out when the security_secid_to_secctx()
>>>> code is brought in line with the lsmblob.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> ---
>>>> include/linux/security.h | 7 +++++--
>>>> include/net/af_unix.h | 2 +-
>>>> include/net/scm.h | 8 +++++---
>>>> net/ipv4/ip_sockglue.c | 8 +++++---
>>>> net/unix/af_unix.c | 6 +++---
>>>> security/security.c | 18 +++++++++++++++---
>>>> 6 files changed, 34 insertions(+), 15 deletions(-)
>>> ...
>>>
>>>> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
>>>> index f42fdddecd41..a86da0cb5ec1 100644
>>>> --- a/include/net/af_unix.h
>>>> +++ b/include/net/af_unix.h
>>>> @@ -36,7 +36,7 @@ struct unix_skb_parms {
>>>> kgid_t gid;
>>>> struct scm_fp_list *fp; /* Passed files */
>>>> #ifdef CONFIG_SECURITY_NETWORK
>>>> - u32 secid; /* Security ID */
>>>> + struct lsmblob lsmblob; /* Security LSM data */
>>> As mentioned in a previous revision, I remain concerned that this is
>>> going to become a problem due to the size limit on unix_skb_parms. I
>>> would need to redo the math to be certain, but if I recall correctly
>>> this would limit us to five LSMs assuming both that we don't need to
>>> grow the per-LSM size of lsmblob *and* the netdev folks don't decide
>>> to add more fields to the unix_skb_parms.
>>>
>>> I lost track of that earlier discussion so I'm not sure where it ended
>>> up, but if there is a viable alternative it might be a good idea to
>>> pursue it.
>> Stephen had concerns about the lifecycle management involved. He also
>> pointed out that I had taken a cowards way out when allocations failed.
>> That could result in unexpected behavior when an allocation failed.
>> Fixing that would have required a major re-write of the currently simple
>> UDS attribute code, which I suspect would be as hard a sell to netdev as
>> expanding the secid to a lsmblob. I also thought I'd gotten netdev on the
>> CC: for this patch, but it looks like I missed it.
>>
>> I did start on the UDS attribute re-do, and discovered that I was going
>> to have to introduce new failure paths, and that it might not be possible
>> to maintain compatibility for all cases because of the new possibilities
>> of failure.
> ... and you're hoping to not be responsible for this code by the time
> this becomes a limiting issue? ;)
Well, maybe. More likely that full dementia will have set in by the
time I get the alternative done correctly. It's a _lot_ more complicated.
I'm carefully watching what the BPF people are doing with their
memory management schemes in the hope they will come up with something
useful.
> I understand the concerns you mention, they are all valid as far as
> I'm concerned, but I think we are going to get burned by this code as
> it currently stands.
Yes, I can see that. We're getting burned by the non-extensibility
of secids. It will take someone smarter than me to figure out how to
fit N secids into 32bits without danger of either failure or memory
allocation.
^ permalink raw reply
* Re: [RFC PATCH] certs: Add EFI_CERT_X509_GUID support for dbx entries]
From: Eric Snowberg @ 2020-09-04 23:20 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dhowells, dwmw2, jmorris, serge, Mimi Zohar, erichte, nayna, mpe,
keyrings, linux-kernel, linux-security-module
In-Reply-To: <20200904125931.GE39023@linux.intel.com>
> On Sep 4, 2020, at 6:59 AM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
> On Tue, Sep 01, 2020 at 12:51:43PM -0400, Eric Snowberg wrote:
>> The Secure Boot Forbidden Signature Database, dbx, contains a list of now
>> revoked signatures and keys previously approved to boot with UEFI Secure
>> Boot enabled. The dbx is capable of containing any number of
>> EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and EFI_CERT_X509_GUID
>> entries.
>>
>> Currently when EFI_CERT_X509_GUID are contained in the dbx, the entries are
>> skipped.
>>
>> This change adds support for EFI_CERT_X509_GUID dbx entries. When a
>> EFI_CERT_X509_GUID is found, it is added as an asymmetrical key to the
>> .blacklist keyring. Anytime the .platform keyring is used, the keys in
>> the .blacklist keyring are referenced, if a matching key is found, the
>> key will be rejected.
>>
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>
> In the last paragraph, please use imperative form: "Add support for …".
I will change this in V2.
>
>> ---
>> certs/blacklist.c | 36 +++++++++++++++++++
>> certs/system_keyring.c | 6 ++++
>> include/keys/system_keyring.h | 11 ++++++
>> .../platform_certs/keyring_handler.c | 11 ++++++
>> 4 files changed, 64 insertions(+)
>>
>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>> index 6514f9ebc943..17ebf50cf0ae 100644
>> --- a/certs/blacklist.c
>> +++ b/certs/blacklist.c
>> @@ -15,6 +15,7 @@
>> #include <linux/err.h>
>> #include <linux/seq_file.h>
>> #include <keys/system_keyring.h>
>> +#include <crypto/pkcs7.h>
>> #include "blacklist.h"
>>
>> static struct key *blacklist_keyring;
>> @@ -100,6 +101,41 @@ int mark_hash_blacklisted(const char *hash)
>> return 0;
>> }
>>
>> +int mark_key_revocationlisted(const char *data, size_t size)
>> +{
>> + key_ref_t key;
>> +
>> + key = key_create_or_update(make_key_ref(blacklist_keyring, true),
>> + "asymmetric",
>> + NULL,
>> + data,
>> + size,
>> + ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>> + KEY_USR_VIEW),
>> + KEY_ALLOC_NOT_IN_QUOTA |
>> + KEY_ALLOC_BUILT_IN);
>> +
>> + if (IS_ERR(key)) {
>> + pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
>> + return PTR_ERR(key);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int is_key_revocationlisted(struct pkcs7_message *pkcs7)
>> +{
>> + int ret;
>> +
>> + ret = pkcs7_validate_trust(pkcs7, blacklist_keyring);
>> +
>> + if (ret == 0)
>> + return -EKEYREJECTED;
>> +
>> + return -ENOKEY;
>> +}
>> +EXPORT_SYMBOL_GPL(is_key_revocationlisted);
>> +
>> /**
>> * is_hash_blacklisted - Determine if a hash is blacklisted
>> * @hash: The hash to be checked as a binary blob
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 798291177186..f8ea96219155 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -241,6 +241,12 @@ int verify_pkcs7_message_sig(const void *data, size_t len,
>> pr_devel("PKCS#7 platform keyring is not available\n");
>> goto error;
>> }
>> +
>> + ret = is_key_revocationlisted(pkcs7);
>> + if (ret != -ENOKEY) {
>> + pr_devel("PKCS#7 platform key revocationlisted\n");
>> + goto error;
>> + }
>> }
>> ret = pkcs7_validate_trust(pkcs7, trusted_keys);
>> if (ret < 0) {
>> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
>> index fb8b07daa9d1..b6991cfe1b6d 100644
>> --- a/include/keys/system_keyring.h
>> +++ b/include/keys/system_keyring.h
>> @@ -31,11 +31,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
>> #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
>> #endif
>>
>> +extern struct pkcs7_message *pkcs7;
>> #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
>> extern int mark_hash_blacklisted(const char *hash);
>> +extern int mark_key_revocationlisted(const char *data, size_t size);
>> extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
>> const char *type);
>> extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
>> +extern int is_key_revocationlisted(struct pkcs7_message *pkcs7);
>> #else
>> static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
>> const char *type)
>> @@ -47,6 +50,14 @@ static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
>> {
>> return 0;
>> }
>> +static inline int mark_key_revocationlisted(const char *data, size_t size)
>> +{
>> + return 0;
>> +}
>> +static inline int is_key_revocationlisted(struct pkcs7_message *pkcs7)
>> +{
>> + return -ENOKEY;
>> +}
>> #endif
>>
>> #ifdef CONFIG_IMA_BLACKLIST_KEYRING
>> diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
>> index c5ba695c10e3..cc5a43804bc4 100644
>> --- a/security/integrity/platform_certs/keyring_handler.c
>> +++ b/security/integrity/platform_certs/keyring_handler.c
>> @@ -55,6 +55,15 @@ static __init void uefi_blacklist_binary(const char *source,
>> uefi_blacklist_hash(source, data, len, "bin:", 4);
>> }
>>
>> +/*
>> + * Revocationlist the X509 cert
>> + */
>> +static __init void uefi_revocationlist_x509(const char *source,
>> + const void *data, size_t len)
>> +{
>> + mark_key_revocationlisted(data, len);
>> +}
>> +
>> /*
>> * Return the appropriate handler for particular signature list types found in
>> * the UEFI db and MokListRT tables.
>> @@ -76,5 +85,7 @@ __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
>> return uefi_blacklist_x509_tbs;
>> if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
>> return uefi_blacklist_binary;
>> + if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
>> + return uefi_revocationlist_x509;
>> return 0;
>> }
>> --
>> 2.18.1
>>
>
> I did not find anything wrong with the code change.
Thanks
^ permalink raw reply
* Re: [PATCH v20 07/23] LSM: Use lsmblob in security_secid_to_secctx
From: Paul Moore @ 2020-09-04 21:59 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <20200826145247.10029-8-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:09 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change security_secid_to_secctx() to take a lsmblob as input
> instead of a u32 secid. It will then call the LSM hooks
> using the lsmblob element allocated for that module. The
> callers have been updated as well. This allows for the
> possibility that more than one module may be called upon
> to translate a secid to a string, as can occur in the
> audit code.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> drivers/android/binder.c | 12 +++++++++-
> include/linux/security.h | 5 +++--
> include/net/scm.h | 8 ++-----
> kernel/audit.c | 20 +++++++++++++++--
> kernel/auditsc.c | 28 +++++++++++++++++++----
> net/ipv4/ip_sockglue.c | 5 +----
> net/netfilter/nf_conntrack_netlink.c | 14 ++++++++++--
> net/netfilter/nf_conntrack_standalone.c | 4 +++-
> net/netfilter/nfnetlink_queue.c | 11 +++++++--
> net/netlabel/netlabel_unlabeled.c | 30 +++++++++++++++++++++----
> net/netlabel/netlabel_user.c | 6 ++---
> security/security.c | 11 +++++----
> 12 files changed, 117 insertions(+), 37 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: Paul Moore @ 2020-09-04 21:53 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <1eeef766-405f-3800-c0cf-3eb008f9673e@schaufler-ca.com>
On Fri, Sep 4, 2020 at 5:35 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 9/4/2020 1:08 PM, Paul Moore wrote:
> > On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> Change the data used in UDS SO_PEERSEC processing from a
> >> secid to a more general struct lsmblob. Update the
> >> security_socket_getpeersec_dgram() interface to use the
> >> lsmblob. There is a small amount of scaffolding code
> >> that will come out when the security_secid_to_secctx()
> >> code is brought in line with the lsmblob.
> >>
> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> >> ---
> >> include/linux/security.h | 7 +++++--
> >> include/net/af_unix.h | 2 +-
> >> include/net/scm.h | 8 +++++---
> >> net/ipv4/ip_sockglue.c | 8 +++++---
> >> net/unix/af_unix.c | 6 +++---
> >> security/security.c | 18 +++++++++++++++---
> >> 6 files changed, 34 insertions(+), 15 deletions(-)
> > ...
> >
> >> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> >> index f42fdddecd41..a86da0cb5ec1 100644
> >> --- a/include/net/af_unix.h
> >> +++ b/include/net/af_unix.h
> >> @@ -36,7 +36,7 @@ struct unix_skb_parms {
> >> kgid_t gid;
> >> struct scm_fp_list *fp; /* Passed files */
> >> #ifdef CONFIG_SECURITY_NETWORK
> >> - u32 secid; /* Security ID */
> >> + struct lsmblob lsmblob; /* Security LSM data */
> > As mentioned in a previous revision, I remain concerned that this is
> > going to become a problem due to the size limit on unix_skb_parms. I
> > would need to redo the math to be certain, but if I recall correctly
> > this would limit us to five LSMs assuming both that we don't need to
> > grow the per-LSM size of lsmblob *and* the netdev folks don't decide
> > to add more fields to the unix_skb_parms.
> >
> > I lost track of that earlier discussion so I'm not sure where it ended
> > up, but if there is a viable alternative it might be a good idea to
> > pursue it.
>
> Stephen had concerns about the lifecycle management involved. He also
> pointed out that I had taken a cowards way out when allocations failed.
> That could result in unexpected behavior when an allocation failed.
> Fixing that would have required a major re-write of the currently simple
> UDS attribute code, which I suspect would be as hard a sell to netdev as
> expanding the secid to a lsmblob. I also thought I'd gotten netdev on the
> CC: for this patch, but it looks like I missed it.
>
> I did start on the UDS attribute re-do, and discovered that I was going
> to have to introduce new failure paths, and that it might not be possible
> to maintain compatibility for all cases because of the new possibilities
> of failure.
... and you're hoping to not be responsible for this code by the time
this becomes a limiting issue? ;)
I understand the concerns you mention, they are all valid as far as
I'm concerned, but I think we are going to get burned by this code as
it currently stands.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 02/23] LSM: Create and manage the lsmblob data structure.
From: Paul Moore @ 2020-09-04 21:50 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <20200826145247.10029-3-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:03 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
>
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
>
> The audit rules data is expanded to use an array of
> security module data rather than a single instance.
> Because IMA uses the audit rule functions it is
> affected as well.
>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/audit.h | 4 +-
> include/linux/lsm_hooks.h | 12 ++++-
> include/linux/security.h | 67 +++++++++++++++++++++++++--
> kernel/auditfilter.c | 24 +++++-----
> kernel/auditsc.c | 12 ++---
> security/apparmor/lsm.c | 7 ++-
> security/bpf/hooks.c | 12 ++++-
> security/commoncap.c | 7 ++-
> security/integrity/ima/ima_policy.c | 40 +++++++++++-----
> security/loadpin/loadpin.c | 8 +++-
> security/lockdown/lockdown.c | 7 ++-
> security/safesetid/lsm.c | 8 +++-
> security/security.c | 72 ++++++++++++++++++++++++-----
> security/selinux/hooks.c | 8 +++-
> security/smack/smack_lsm.c | 7 ++-
> security/tomoyo/tomoyo.c | 8 +++-
> security/yama/yama_lsm.c | 7 ++-
> 17 files changed, 254 insertions(+), 56 deletions(-)
...
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0a0a03b36a3b..c91389d7aebc 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -131,6 +131,65 @@ enum lockdown_reason {
>
> extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
>
> +/*
> + * Data exported by the security modules
> + *
> + * Any LSM that provides secid or secctx based hooks must be included.
> + */
> +#define LSMBLOB_ENTRIES ( \
> + (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
> + (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
> +
> +struct lsmblob {
> + u32 secid[LSMBLOB_ENTRIES];
> +};
> +
> +#define LSMBLOB_INVALID -1 /* Not a valid LSM slot number */
> +#define LSMBLOB_NEEDED -2 /* Slot requested on initialization */
> +#define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> +
> +/**
> + * lsmblob_init - initialize an lsmblob structure.
> + * @blob: Pointer to the data to initialize
> + * @secid: The initial secid value
> + *
> + * Set all secid for all modules to the specified value.
> + */
> +static inline void lsmblob_init(struct lsmblob *blob, u32 secid)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + blob->secid[i] = secid;
> +}
As I'm going through the v20 draft of these patches it occurs to me,
at least in the intermediate patches, that there is a pretty common
pattern involving lsmblob_init():
lsmblob_init(blob, secid);
func(blob, ...);
... would it make sense to have lsmblob_init() return *blob instead of
void? It doesn't really matter too much, but it seems like it could
help cleanup some of the code:
func(lsmblob_init(blob, secid), ...);
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: Casey Schaufler @ 2020-09-04 21:35 UTC (permalink / raw)
To: Paul Moore
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, Casey Schaufler
In-Reply-To: <CAHC9VhSh=r4w_3mZOUwmKN0UxCMxPNGKd=_vr_iGV06rvCNbSA@mail.gmail.com>
On 9/4/2020 1:08 PM, Paul Moore wrote:
> On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Change the data used in UDS SO_PEERSEC processing from a
>> secid to a more general struct lsmblob. Update the
>> security_socket_getpeersec_dgram() interface to use the
>> lsmblob. There is a small amount of scaffolding code
>> that will come out when the security_secid_to_secctx()
>> code is brought in line with the lsmblob.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>> include/linux/security.h | 7 +++++--
>> include/net/af_unix.h | 2 +-
>> include/net/scm.h | 8 +++++---
>> net/ipv4/ip_sockglue.c | 8 +++++---
>> net/unix/af_unix.c | 6 +++---
>> security/security.c | 18 +++++++++++++++---
>> 6 files changed, 34 insertions(+), 15 deletions(-)
> ...
>
>> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
>> index f42fdddecd41..a86da0cb5ec1 100644
>> --- a/include/net/af_unix.h
>> +++ b/include/net/af_unix.h
>> @@ -36,7 +36,7 @@ struct unix_skb_parms {
>> kgid_t gid;
>> struct scm_fp_list *fp; /* Passed files */
>> #ifdef CONFIG_SECURITY_NETWORK
>> - u32 secid; /* Security ID */
>> + struct lsmblob lsmblob; /* Security LSM data */
> As mentioned in a previous revision, I remain concerned that this is
> going to become a problem due to the size limit on unix_skb_parms. I
> would need to redo the math to be certain, but if I recall correctly
> this would limit us to five LSMs assuming both that we don't need to
> grow the per-LSM size of lsmblob *and* the netdev folks don't decide
> to add more fields to the unix_skb_parms.
>
> I lost track of that earlier discussion so I'm not sure where it ended
> up, but if there is a viable alternative it might be a good idea to
> pursue it.
Stephen had concerns about the lifecycle management involved. He also
pointed out that I had taken a cowards way out when allocations failed.
That could result in unexpected behavior when an allocation failed.
Fixing that would have required a major re-write of the currently simple
UDS attribute code, which I suspect would be as hard a sell to netdev as
expanding the secid to a lsmblob. I also thought I'd gotten netdev on the
CC: for this patch, but it looks like I missed it.
I did start on the UDS attribute re-do, and discovered that I was going
to have to introduce new failure paths, and that it might not be possible
to maintain compatibility for all cases because of the new possibilities
of failure.
^ permalink raw reply
* Re: [PATCH v20 06/23] LSM: Use lsmblob in security_secctx_to_secid
From: Paul Moore @ 2020-09-04 21:29 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, netdev
In-Reply-To: <20200826145247.10029-7-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:08 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> Change security_secctx_to_secid() to fill in a lsmblob instead
> of a u32 secid. Multiple LSMs may be able to interpret the
> string, and this allows for setting whichever secid is
> appropriate. Change security_secmark_relabel_packet() to use a
> lsmblob instead of a u32 secid. In some other cases there is
> scaffolding where interfaces have yet to be converted.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: netdev@vger.kernel.org
> ---
> include/linux/security.h | 30 +++++++++++++++++++++++----
> include/net/scm.h | 7 +++++--
> kernel/cred.c | 4 +---
> net/ipv4/ip_sockglue.c | 6 ++++--
> net/netfilter/nft_meta.c | 18 +++++++++-------
> net/netfilter/xt_SECMARK.c | 9 ++++++--
> net/netlabel/netlabel_unlabeled.c | 23 +++++++++++++--------
> security/security.c | 34 ++++++++++++++++++++++++++-----
> 8 files changed, 98 insertions(+), 33 deletions(-)
I imagine there may be ways around the xt_secmark_target_info
limitation, but that would require userspace changes to take advantage
of it, and the way forward is clearly nftables so it probably isn't
worth the effort.
I'm okay with this patch with the understanding that several chunks in
the patch are replaced by later patches in the series.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ae623b89cdf4..f8770c228356 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -190,6 +190,27 @@ static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
> return !memcmp(bloba, blobb, sizeof(*bloba));
> }
>
> +/**
> + * lsmblob_value - find the first non-zero value in an lsmblob structure.
> + * @blob: Pointer to the data
> + *
> + * This needs to be used with extreme caution, as the cases where
> + * it is appropriate are rare.
> + *
> + * Return the first secid value set in the lsmblob.
> + * There should only be one.
> + */
> +static inline u32 lsmblob_value(const struct lsmblob *blob)
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (blob->secid[i])
> + return blob->secid[i];
> +
> + return 0;
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> @@ -503,7 +524,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> + struct lsmblob *blob);
> void security_release_secctx(char *secdata, u32 seclen);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1322,7 +1344,7 @@ static inline int security_secid_to_secctx(u32 secid, char **secdata, u32 *secle
>
> static inline int security_secctx_to_secid(const char *secdata,
> u32 seclen,
> - u32 *secid)
> + struct lsmblob *blob)
> {
> return -EOPNOTSUPP;
> }
> @@ -1412,7 +1434,7 @@ void security_inet_csk_clone(struct sock *newsk,
> const struct request_sock *req);
> void security_inet_conn_established(struct sock *sk,
> struct sk_buff *skb);
> -int security_secmark_relabel_packet(u32 secid);
> +int security_secmark_relabel_packet(struct lsmblob *blob);
> void security_secmark_refcount_inc(void);
> void security_secmark_refcount_dec(void);
> int security_tun_dev_alloc_security(void **security);
> @@ -1585,7 +1607,7 @@ static inline void security_inet_conn_established(struct sock *sk,
> {
> }
>
> -static inline int security_secmark_relabel_packet(u32 secid)
> +static inline int security_secmark_relabel_packet(struct lsmblob *blob)
> {
> return 0;
> }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index e2e71c4bf9d0..c09f2dfeec88 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -97,8 +97,11 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
> int err;
>
> if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> - /* Scaffolding - it has to be element 0 for now */
> - err = security_secid_to_secctx(scm->lsmblob.secid[0],
> + /* There can currently be only one value in the lsmblob,
> + * so getting it from lsmblob_value is appropriate until
> + * security_secid_to_secctx() is converted to taking a
> + * lsmblob directly. */
> + err = security_secid_to_secctx(lsmblob_value(&scm->lsmblob),
> &secdata, &seclen);
>
> if (!err) {
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 22e0e7cbefde..848306c7d823 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -757,14 +757,12 @@ EXPORT_SYMBOL(set_security_override);
> int set_security_override_from_ctx(struct cred *new, const char *secctx)
> {
> struct lsmblob blob;
> - u32 secid;
> int ret;
>
> - ret = security_secctx_to_secid(secctx, strlen(secctx), &secid);
> + ret = security_secctx_to_secid(secctx, strlen(secctx), &blob);
> if (ret < 0)
> return ret;
>
> - lsmblob_init(&blob, secid);
> return set_security_override(new, &blob);
> }
> EXPORT_SYMBOL(set_security_override_from_ctx);
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 551dfbc717e9..c568574abfae 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -139,8 +139,10 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
> if (err)
> return;
>
> - /* Scaffolding - it has to be element 0 */
> - err = security_secid_to_secctx(lb.secid[0], &secdata, &seclen);
> + /* There can only be one secid in the lsmblob at this point,
> + * so getting it using lsmblob_value() is sufficient until
> + * security_secid_to_secctx() is changed to use a lsmblob */
> + err = security_secid_to_secctx(lsmblob_value(&lb), &secdata, &seclen);
> if (err)
> return;
>
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 7bc6537f3ccb..7db487d93618 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -801,7 +801,7 @@ struct nft_expr_type nft_meta_type __read_mostly = {
>
> #ifdef CONFIG_NETWORK_SECMARK
> struct nft_secmark {
> - u32 secid;
> + struct lsmblob lsmdata;
> char *ctx;
> };
>
> @@ -811,21 +811,21 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
>
> static int nft_secmark_compute_secid(struct nft_secmark *priv)
> {
> - u32 tmp_secid = 0;
> + struct lsmblob blob;
> int err;
>
> - err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid);
> + err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob);
> if (err)
> return err;
>
> - if (!tmp_secid)
> + if (!lsmblob_is_set(&blob))
> return -ENOENT;
>
> - err = security_secmark_relabel_packet(tmp_secid);
> + err = security_secmark_relabel_packet(&blob);
> if (err)
> return err;
>
> - priv->secid = tmp_secid;
> + priv->lsmdata = blob;
> return 0;
> }
>
> @@ -835,7 +835,11 @@ static void nft_secmark_obj_eval(struct nft_object *obj, struct nft_regs *regs,
> const struct nft_secmark *priv = nft_obj_data(obj);
> struct sk_buff *skb = pkt->skb;
>
> - skb->secmark = priv->secid;
> + /* It is not possible for more than one secid to be set in
> + * the lsmblob structure because it is set using
> + * security_secctx_to_secid(). Any secid that is set must therefore
> + * be the one that should go in the secmark. */
> + skb->secmark = lsmblob_value(&priv->lsmdata);
> }
>
> static int nft_secmark_obj_init(const struct nft_ctx *ctx,
> diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
> index 75625d13e976..5a268707eeda 100644
> --- a/net/netfilter/xt_SECMARK.c
> +++ b/net/netfilter/xt_SECMARK.c
> @@ -43,13 +43,14 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
>
> static int checkentry_lsm(struct xt_secmark_target_info *info)
> {
> + struct lsmblob blob;
> int err;
>
> info->secctx[SECMARK_SECCTX_MAX - 1] = '\0';
> info->secid = 0;
>
> err = security_secctx_to_secid(info->secctx, strlen(info->secctx),
> - &info->secid);
> + &blob);
> if (err) {
> if (err == -EINVAL)
> pr_info_ratelimited("invalid security context \'%s\'\n",
> @@ -57,13 +58,17 @@ static int checkentry_lsm(struct xt_secmark_target_info *info)
> return err;
> }
>
> + /* xt_secmark_target_info can't be changed to use lsmblobs because
> + * it is exposed as an API. Use lsmblob_value() to get the one
> + * value that got set by security_secctx_to_secid(). */
> + info->secid = lsmblob_value(&blob);
> if (!info->secid) {
> pr_info_ratelimited("unable to map security context \'%s\'\n",
> info->secctx);
> return -ENOENT;
> }
>
> - err = security_secmark_relabel_packet(info->secid);
> + err = security_secmark_relabel_packet(&blob);
> if (err) {
> pr_info_ratelimited("unable to obtain relabeling permission\n");
> return err;
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 77bb1bb22c3b..8948557eaebb 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -882,7 +882,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> void *addr;
> void *mask;
> u32 addr_len;
> - u32 secid;
> + struct lsmblob blob;
> struct netlbl_audit audit_info;
>
> /* Don't allow users to add both IPv4 and IPv6 addresses for a
> @@ -906,13 +906,18 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> ret_val = security_secctx_to_secid(
> nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> - &secid);
> + &blob);
> if (ret_val != 0)
> return ret_val;
>
> + /* netlbl_unlhsh_add will be changed to pass a struct lsmblob *
> + * instead of a u32 later in this patch set. security_secctx_to_secid()
> + * will only be setting one entry in the lsmblob struct, so it is
> + * safe to use lsmblob_value() to get that one value. */
> +
> return netlbl_unlhsh_add(&init_net,
> - dev_name, addr, mask, addr_len, secid,
> - &audit_info);
> + dev_name, addr, mask, addr_len,
> + lsmblob_value(&blob), &audit_info);
> }
>
> /**
> @@ -933,7 +938,7 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> void *addr;
> void *mask;
> u32 addr_len;
> - u32 secid;
> + struct lsmblob blob;
> struct netlbl_audit audit_info;
>
> /* Don't allow users to add both IPv4 and IPv6 addresses for a
> @@ -955,13 +960,15 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> ret_val = security_secctx_to_secid(
> nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> - &secid);
> + &blob);
> if (ret_val != 0)
> return ret_val;
>
> + /* security_secctx_to_secid() will only put one secid into the lsmblob
> + * so it's safe to use lsmblob_value() to get the secid. */
> return netlbl_unlhsh_add(&init_net,
> - NULL, addr, mask, addr_len, secid,
> - &audit_info);
> + NULL, addr, mask, addr_len,
> + lsmblob_value(&blob), &audit_info);
> }
>
> /**
> diff --git a/security/security.c b/security/security.c
> index c42873876954..5c2ed1db0658 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2065,10 +2065,22 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
>
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> + struct lsmblob *blob)
> {
> - *secid = 0;
> - return call_int_hook(secctx_to_secid, 0, secdata, seclen, secid);
> + struct security_hook_list *hp;
> + int rc;
> +
> + lsmblob_init(blob, 0);
> + hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> + continue;
> + rc = hp->hook.secctx_to_secid(secdata, seclen,
> + &blob->secid[hp->lsmid->slot]);
> + if (rc != 0)
> + return rc;
> + }
> + return 0;
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
> @@ -2301,9 +2313,21 @@ void security_inet_conn_established(struct sock *sk,
> }
> EXPORT_SYMBOL(security_inet_conn_established);
>
> -int security_secmark_relabel_packet(u32 secid)
> +int security_secmark_relabel_packet(struct lsmblob *blob)
> {
> - return call_int_hook(secmark_relabel_packet, 0, secid);
> + struct security_hook_list *hp;
> + int rc = 0;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.secmark_relabel_packet,
> + list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> + continue;
> + rc = hp->hook.secmark_relabel_packet(
> + blob->secid[hp->lsmid->slot]);
> + if (rc != 0)
> + break;
> + }
> + return rc;
> }
> EXPORT_SYMBOL(security_secmark_relabel_packet);
>
> --
> 2.24.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 05/23] net: Prepare UDS for security module stacking
From: Paul Moore @ 2020-09-04 20:08 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <20200826145247.10029-6-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:07 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/security.h | 7 +++++--
> include/net/af_unix.h | 2 +-
> include/net/scm.h | 8 +++++---
> net/ipv4/ip_sockglue.c | 8 +++++---
> net/unix/af_unix.c | 6 +++---
> security/security.c | 18 +++++++++++++++---
> 6 files changed, 34 insertions(+), 15 deletions(-)
...
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index f42fdddecd41..a86da0cb5ec1 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -36,7 +36,7 @@ struct unix_skb_parms {
> kgid_t gid;
> struct scm_fp_list *fp; /* Passed files */
> #ifdef CONFIG_SECURITY_NETWORK
> - u32 secid; /* Security ID */
> + struct lsmblob lsmblob; /* Security LSM data */
As mentioned in a previous revision, I remain concerned that this is
going to become a problem due to the size limit on unix_skb_parms. I
would need to redo the math to be certain, but if I recall correctly
this would limit us to five LSMs assuming both that we don't need to
grow the per-LSM size of lsmblob *and* the netdev folks don't decide
to add more fields to the unix_skb_parms.
I lost track of that earlier discussion so I'm not sure where it ended
up, but if there is a viable alternative it might be a good idea to
pursue it.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 04/23] LSM: Use lsmblob in security_kernel_act_as
From: Paul Moore @ 2020-09-04 19:46 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <20200826145247.10029-5-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:05 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the security_kernel_act_as interface to use a lsmblob
> structure in place of the single u32 secid in support of
> module stacking. Change its only caller, set_security_override,
> to do the same. Change that one's only caller,
> set_security_override_from_ctx, to call it with the new
> parameter type.
>
> The security module hook is unchanged, still taking a secid.
> The infrastructure passes the correct entry from the lsmblob.
> lsmblob_init() is used to fill the lsmblob structure, however
> this will be removed later in the series when security_secctx_to_secid()
> is undated to provide a lsmblob instead of a secid.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/cred.h | 3 ++-
> include/linux/security.h | 5 +++--
> kernel/cred.c | 10 ++++++----
> security/security.c | 14 ++++++++++++--
> 4 files changed, 23 insertions(+), 9 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v20 03/23] LSM: Use lsmblob in security_audit_rule_match
From: Paul Moore @ 2020-09-04 18:53 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley
In-Reply-To: <20200826145247.10029-4-casey@schaufler-ca.com>
On Wed, Aug 26, 2020 at 11:04 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Change the secid parameter of security_audit_rule_match
> to a lsmblob structure pointer. Pass the entry from the
> lsmblob structure for the approprite slot to the LSM hook.
>
> Change the users of security_audit_rule_match to use the
> lsmblob instead of a u32. The scaffolding function lsmblob_init()
> fills the blob with the value of the old secid, ensuring that
> it is available to the appropriate module hook. The sources of
> the secid, security_task_getsecid() and security_inode_getsecid(),
> will be converted to use the blob structure later in the series.
> At the point the use of lsmblob_init() is dropped.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/security.h | 7 ++++---
> kernel/auditfilter.c | 6 ++++--
> kernel/auditsc.c | 14 ++++++++++----
> security/integrity/ima/ima.h | 4 ++--
> security/integrity/ima/ima_policy.c | 7 +++++--
> security/security.c | 10 ++++++++--
> 6 files changed, 33 insertions(+), 15 deletions(-)
Acked-by: Paul Moore <paul@paul-moore.com>
--
paul moore
www.paul-moore.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox