* Re: [PATCH v5] KEYS: encrypted: Instantiate key with user-provided decrypted data
From: Sumit Garg @ 2022-02-21 5:47 UTC (permalink / raw)
To: Yael Tzur
Cc: linux-integrity, jejb, jarkko, zohar, corbet, dhowells, jmorris,
serge, keyrings, linux-doc, linux-kernel, linux-security-module
In-Reply-To: <20220215141953.1557009-1-yaelt@google.com>
On Tue, 15 Feb 2022 at 19:50, Yael Tzur <yaelt@google.com> wrote:
>
> For availability and performance reasons master keys often need to be
> released outside of a Key Management Service (KMS) to clients. It
> would be beneficial to provide a mechanism where the
> wrapping/unwrapping of data encryption keys (DEKs) is not dependent
> on a remote call at runtime yet security is not (or only minimally)
> compromised. Master keys could be securely stored in the Kernel and
> be used to wrap/unwrap keys from Userspace.
>
> The encrypted.c class supports instantiation of encrypted keys with
> either an already-encrypted key material, or by generating new key
> material based on random numbers. This patch defines a new datablob
> format: [<format>] <master-key name> <decrypted data length>
> <decrypted data> that allows to inject and encrypt user-provided
> decrypted data. The decrypted data must be hex-ascii encoded.
>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Yael Tzur <yaelt@google.com>
> ---
>
> Notes:
> v -> v2: fixed compilation error.
>
> v2 -> v3: modified documentation.
>
> v3 -> v4: modified commit message.
>
> v4 -> v5: added config option to enable feature, and modified input validation.
>
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
-Sumit
> .../security/keys/trusted-encrypted.rst | 25 +++++--
> security/keys/Kconfig | 19 +++--
> security/keys/encrypted-keys/encrypted.c | 72 ++++++++++++++-----
> 3 files changed, 87 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/security/keys/trusted-encrypted.rst b/Documentation/security/keys/trusted-encrypted.rst
> index 80d5a5af62a1..f614dad7de12 100644
> --- a/Documentation/security/keys/trusted-encrypted.rst
> +++ b/Documentation/security/keys/trusted-encrypted.rst
> @@ -107,12 +107,13 @@ Encrypted Keys
> --------------
>
> Encrypted keys do not depend on a trust source, and are faster, as they use AES
> -for encryption/decryption. New keys are created from kernel-generated random
> -numbers, and are encrypted/decrypted using a specified ‘master’ key. The
> -‘master’ key can either be a trusted-key or user-key type. The main disadvantage
> -of encrypted keys is that if they are not rooted in a trusted key, they are only
> -as secure as the user key encrypting them. The master user key should therefore
> -be loaded in as secure a way as possible, preferably early in boot.
> +for encryption/decryption. New keys are created either from kernel-generated
> +random numbers or user-provided decrypted data, and are encrypted/decrypted
> +using a specified ‘master’ key. The ‘master’ key can either be a trusted-key or
> +user-key type. The main disadvantage of encrypted keys is that if they are not
> +rooted in a trusted key, they are only as secure as the user key encrypting
> +them. The master user key should therefore be loaded in as secure a way as
> +possible, preferably early in boot.
>
>
> Usage
> @@ -199,6 +200,8 @@ Usage::
>
> keyctl add encrypted name "new [format] key-type:master-key-name keylen"
> ring
> + keyctl add encrypted name "new [format] key-type:master-key-name keylen
> + decrypted-data" ring
> keyctl add encrypted name "load hex_blob" ring
> keyctl update keyid "update key-type:master-key-name"
>
> @@ -303,6 +306,16 @@ Load an encrypted key "evm" from saved blob::
> 82dbbc55be2a44616e4959430436dc4f2a7a9659aa60bb4652aeb2120f149ed197c564e0
> 24717c64 5972dcb82ab2dde83376d82b2e3c09ffc
>
> +Instantiate an encrypted key "evm" using user-provided decrypted data::
> +
> + $ keyctl add encrypted evm "new default user:kmk 32 `cat evm_decrypted_data.blob`" @u
> + 794890253
> +
> + $ keyctl print 794890253
> + default user:kmk 32 2375725ad57798846a9bbd240de8906f006e66c03af53b1b382d
> + bbc55be2a44616e4959430436dc4f2a7a9659aa60bb4652aeb2120f149ed197c564e0247
> + 17c64 5972dcb82ab2dde83376d82b2e3c09ffc
> +
> Other uses for trusted and encrypted keys, such as for disk and file encryption
> are anticipated. In particular the new format 'ecryptfs' has been defined
> in order to use encrypted keys to mount an eCryptfs filesystem. More details
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 969122c7b92f..0e30b361e1c1 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -98,10 +98,21 @@ config ENCRYPTED_KEYS
> select CRYPTO_RNG
> help
> This option provides support for create/encrypting/decrypting keys
> - in the kernel. Encrypted keys are kernel generated random numbers,
> - which are encrypted/decrypted with a 'master' symmetric key. The
> - 'master' key can be either a trusted-key or user-key type.
> - Userspace only ever sees/stores encrypted blobs.
> + in the kernel. Encrypted keys are instantiated using kernel
> + generated random numbers or provided decrypted data, and are
> + encrypted/decrypted with a 'master' symmetric key. The 'master'
> + key can be either a trusted-key or user-key type. Only encrypted
> + blobs are ever output to Userspace.
> +
> + If you are unsure as to whether this is required, answer N.
> +
> +config USER_DECRYPTED_DATA
> + bool "Allow encrypted keys with user decrypted data"
> + depends on ENCRYPTED_KEYS
> + help
> + This option provides support for instantiating encrypted keys using
> + user-provided decrypted data. The decrypted data must be hex-ascii
> + encoded.
>
> If you are unsure as to whether this is required, answer N.
>
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index 87432b35d771..ebfb8129fb92 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -78,6 +78,11 @@ static const match_table_t key_tokens = {
> {Opt_err, NULL}
> };
>
> +static bool user_decrypted_data = IS_ENABLED(CONFIG_USER_DECRYPTED_DATA);
> +module_param(user_decrypted_data, bool, 0);
> +MODULE_PARM_DESC(user_decrypted_data,
> + "Allow instantiation of encrypted keys using provided decrypted data");
> +
> static int aes_get_sizes(void)
> {
> struct crypto_skcipher *tfm;
> @@ -158,7 +163,7 @@ static int valid_master_desc(const char *new_desc, const char *orig_desc)
> * datablob_parse - parse the keyctl data
> *
> * datablob format:
> - * new [<format>] <master-key name> <decrypted data length>
> + * new [<format>] <master-key name> <decrypted data length> [<decrypted data>]
> * load [<format>] <master-key name> <decrypted data length>
> * <encrypted iv + data>
> * update <new-master-key name>
> @@ -170,7 +175,7 @@ static int valid_master_desc(const char *new_desc, const char *orig_desc)
> */
> static int datablob_parse(char *datablob, const char **format,
> char **master_desc, char **decrypted_datalen,
> - char **hex_encoded_iv)
> + char **hex_encoded_iv, char **decrypted_data)
> {
> substring_t args[MAX_OPT_ARGS];
> int ret = -EINVAL;
> @@ -231,6 +236,7 @@ static int datablob_parse(char *datablob, const char **format,
> "when called from .update method\n", keyword);
> break;
> }
> + *decrypted_data = strsep(&datablob, " \t");
> ret = 0;
> break;
> case Opt_load:
> @@ -595,7 +601,8 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload,
> static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> const char *format,
> const char *master_desc,
> - const char *datalen)
> + const char *datalen,
> + const char *decrypted_data)
> {
> struct encrypted_key_payload *epayload = NULL;
> unsigned short datablob_len;
> @@ -604,6 +611,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> unsigned int encrypted_datalen;
> unsigned int format_len;
> long dlen;
> + int i;
> int ret;
>
> ret = kstrtol(datalen, 10, &dlen);
> @@ -613,6 +620,24 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> format_len = (!format) ? strlen(key_format_default) : strlen(format);
> decrypted_datalen = dlen;
> payload_datalen = decrypted_datalen;
> +
> + if (decrypted_data) {
> + if (!user_decrypted_data) {
> + pr_err("encrypted key: instantiation of keys using provided decrypted data is disabled since CONFIG_USER_DECRYPTED_DATA is set to false\n");
> + return ERR_PTR(-EINVAL);
> + }
> + if (strlen(decrypted_data) != decrypted_datalen) {
> + pr_err("encrypted key: decrypted data provided does not match decrypted data length provided\n");
> + return ERR_PTR(-EINVAL);
> + }
> + for (i = 0; i < strlen(decrypted_data); i++) {
> + if (!isxdigit(decrypted_data[i])) {
> + pr_err("encrypted key: decrypted data provided must contain only hexadecimal characters\n");
> + return ERR_PTR(-EINVAL);
> + }
> + }
> + }
> +
> if (format) {
> if (!strcmp(format, key_format_ecryptfs)) {
> if (dlen != ECRYPTFS_MAX_KEY_BYTES) {
> @@ -740,13 +766,14 @@ static void __ekey_init(struct encrypted_key_payload *epayload,
> /*
> * encrypted_init - initialize an encrypted key
> *
> - * For a new key, use a random number for both the iv and data
> - * itself. For an old key, decrypt the hex encoded data.
> + * For a new key, use either a random number or user-provided decrypted data in
> + * case it is provided. A random number is used for the iv in both cases. For
> + * an old key, decrypt the hex encoded data.
> */
> static int encrypted_init(struct encrypted_key_payload *epayload,
> const char *key_desc, const char *format,
> const char *master_desc, const char *datalen,
> - const char *hex_encoded_iv)
> + const char *hex_encoded_iv, const char *decrypted_data)
> {
> int ret = 0;
>
> @@ -760,21 +787,26 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
> }
>
> __ekey_init(epayload, format, master_desc, datalen);
> - if (!hex_encoded_iv) {
> - get_random_bytes(epayload->iv, ivsize);
> -
> - get_random_bytes(epayload->decrypted_data,
> - epayload->decrypted_datalen);
> - } else
> + if (hex_encoded_iv) {
> ret = encrypted_key_decrypt(epayload, format, hex_encoded_iv);
> + } else if (decrypted_data) {
> + get_random_bytes(epayload->iv, ivsize);
> + memcpy(epayload->decrypted_data, decrypted_data,
> + epayload->decrypted_datalen);
> + } else {
> + get_random_bytes(epayload->iv, ivsize);
> + get_random_bytes(epayload->decrypted_data, epayload->decrypted_datalen);
> + }
> return ret;
> }
>
> /*
> * encrypted_instantiate - instantiate an encrypted key
> *
> - * Decrypt an existing encrypted datablob or create a new encrypted key
> - * based on a kernel random number.
> + * Instantiates the key:
> + * - by decrypting an existing encrypted datablob, or
> + * - by creating a new encrypted key based on a kernel random number, or
> + * - using provided decrypted data.
> *
> * On success, return 0. Otherwise return errno.
> */
> @@ -787,6 +819,7 @@ static int encrypted_instantiate(struct key *key,
> char *master_desc = NULL;
> char *decrypted_datalen = NULL;
> char *hex_encoded_iv = NULL;
> + char *decrypted_data = NULL;
> size_t datalen = prep->datalen;
> int ret;
>
> @@ -799,18 +832,18 @@ static int encrypted_instantiate(struct key *key,
> datablob[datalen] = 0;
> memcpy(datablob, prep->data, datalen);
> ret = datablob_parse(datablob, &format, &master_desc,
> - &decrypted_datalen, &hex_encoded_iv);
> + &decrypted_datalen, &hex_encoded_iv, &decrypted_data);
> if (ret < 0)
> goto out;
>
> epayload = encrypted_key_alloc(key, format, master_desc,
> - decrypted_datalen);
> + decrypted_datalen, decrypted_data);
> if (IS_ERR(epayload)) {
> ret = PTR_ERR(epayload);
> goto out;
> }
> ret = encrypted_init(epayload, key->description, format, master_desc,
> - decrypted_datalen, hex_encoded_iv);
> + decrypted_datalen, hex_encoded_iv, decrypted_data);
> if (ret < 0) {
> kfree_sensitive(epayload);
> goto out;
> @@ -860,7 +893,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
>
> buf[datalen] = 0;
> memcpy(buf, prep->data, datalen);
> - ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL);
> + ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL, NULL);
> if (ret < 0)
> goto out;
>
> @@ -869,7 +902,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
> goto out;
>
> new_epayload = encrypted_key_alloc(key, epayload->format,
> - new_master_desc, epayload->datalen);
> + new_master_desc, epayload->datalen, NULL);
> if (IS_ERR(new_epayload)) {
> ret = PTR_ERR(new_epayload);
> goto out;
> --
> 2.35.1.265.g69c8d7142f-goog
>
^ permalink raw reply
* Re: [PATCH v5] KEYS: encrypted: Instantiate key with user-provided decrypted data
From: Mimi Zohar @ 2022-02-21 5:11 UTC (permalink / raw)
To: Yael Tzur, linux-integrity
Cc: jejb, jarkko, corbet, dhowells, jmorris, serge, keyrings,
linux-doc, linux-kernel, linux-security-module
In-Reply-To: <20220215141953.1557009-1-yaelt@google.com>
On Tue, 2022-02-15 at 09:19 -0500, Yael Tzur wrote:
> For availability and performance reasons master keys often need to be
> released outside of a Key Management Service (KMS) to clients. It
> would be beneficial to provide a mechanism where the
> wrapping/unwrapping of data encryption keys (DEKs) is not dependent
> on a remote call at runtime yet security is not (or only minimally)
> compromised. Master keys could be securely stored in the Kernel and
> be used to wrap/unwrap keys from Userspace.
>
> The encrypted.c class supports instantiation of encrypted keys with
> either an already-encrypted key material, or by generating new key
> material based on random numbers. This patch defines a new datablob
> format: [<format>] <master-key name> <decrypted data length>
> <decrypted data> that allows to inject and encrypt user-provided
> decrypted data. The decrypted data must be hex-ascii encoded.
>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Yael Tzur <yaelt@google.com>
Thanks, Yael.
This patch is now queued in the #next-integrity-testing branch.
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: Avoid calling null function trusted_key_exit
From: Jarkko Sakkinen @ 2022-02-21 1:57 UTC (permalink / raw)
To: Dave Kleikamp
Cc: linux-kernel, Sumit Garg, James Bottomley, Mimi Zohar,
David Howells, James Morris, Serge E. Hallyn, linux-integrity,
keyrings, linux-security-module, stable
In-Reply-To: <cda221fd-6481-598f-03ba-29d0b79af8b7@oracle.com>
On Mon, Feb 07, 2022 at 11:40:23AM -0600, Dave Kleikamp wrote:
> On 1/26/22 2:21PM, Jarkko Sakkinen wrote:
> > On Wed, Jan 26, 2022 at 12:41:55PM -0600, Dave Kleikamp wrote:
> > > If one loads and unloads the trusted module, trusted_key_exit can be
> > > NULL. Call it through static_call_cond() to avoid a kernel trap.
> > >
> > > Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> > >
> > > Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
> >
> > Please re-send with cc stable and the empty line removed and I'll pick it.
>
> I re-sent a v2, but haven't seen any response from you.
>
> I can send it again, or feel free to clean up those lines yourself.
>
> Thanks,
> Shaggy
I've applied the patch. Thank you, and apologies for the latency.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v8 00/17] Enroll kernel keys thru MOK
From: Jarkko Sakkinen @ 2022-02-20 23:23 UTC (permalink / raw)
To: Eric Snowberg
Cc: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert, davem,
jmorris, serge, keescook, torvalds, weiyongjun1, nayna, ebiggers,
ardb, nramas, lszubowi, jason, linux-kernel, linux-crypto,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <20211124044124.998170-1-eric.snowberg@oracle.com>
On Tue, Nov 23, 2021 at 11:41:07PM -0500, Eric Snowberg wrote:
> Back in 2013 Linus requested a feature to allow end-users to have the
> ability "to add their own keys and sign modules they trust". This was
> his *second* order outlined here [1]. There have been many attempts
> over the years to solve this problem, all have been rejected. Many
> of the failed attempts loaded all preboot firmware keys into the kernel,
> including the Secure Boot keys. Many distributions carry one of these
> rejected attempts [2], [3], [4]. This series tries to solve this problem
> with a solution that takes into account all the problems brought up in
> the previous attempts.
>
> On UEFI based systems, this series introduces a new Linux kernel keyring
> containing the Machine Owner Keys (MOK) called machine. It also defines
> a new MOK variable in shim. This variable allows the end-user to decide
> if they want to load MOK keys into the machine keyring. Mimi has suggested
> that only CA keys contained within the MOK be loaded into the machine
> keyring. All other certs will load into the platform keyring instead.
>
> By default, nothing changes; MOK keys are not loaded into the machine
> keyring. They are only loaded after the end-user makes the decision
> themselves. The end-user would set this through mokutil using a new
> --trust-mok option [5]. This would work similar to how the kernel uses
> MOK variables to enable/disable signature validation as well as use/ignore
> the db. Any kernel operation that uses either the builtin or secondary
> trusted keys as a trust source shall also reference the new machine
> keyring as a trust source.
>
> Secure Boot keys will never be loaded into the machine keyring. They
> will always be loaded into the platform keyring. If an end-user wanted
> to load one, they would need to enroll it into the MOK.
>
> Steps required by the end user:
>
> Sign kernel module with user created key:
> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
> machine_signing_key.priv machine_signing_key.x509 my_module.ko
>
> Import the key into the MOK
> $ mokutil --import machine_signing_key.x509
>
> Setup the kernel to load MOK keys into the .machine keyring
> $ mokutil --trust-mok
>
> Then reboot, the MokManager will load and ask if you want to trust the
> MOK key and enroll the MOK into the MOKList. Afterwards the signed kernel
> module will load.
>
> I have included a link to the mokutil [5] changes I have made to support
> this new functionality. The shim changes have now been accepted
> upstream [6].
>
> Upstream shim is located here [7], the build instructions are here [8].
> TLDR:
>
> $ git clone --recurse-submodules https://github.com/rhboot/shim
> $ cd shim
> $ make
>
> After building shim, move shimx64.efi and mmx64.efi to the vendor or
> distribution specific directory on your EFI System Partition (assuming
> you are building on x86). The instructions above are the minimal
> steps needed to build shim to test this feature. It is assumed
> Secure Boot shall not be enabled for this testing. To do testing
> with Secure Boot enabled, all steps in the build instructions [8]
> must be followed.
>
> Instructions for building mokutil (including the new changes):
>
> $ git clone -b mokvars-v3 https://github.com/esnowberg/mokutil.git
> $ cd mokutil/
> $ ./autogen.sh
> $ make
>
> [1] https://marc.info/?l=linux-kernel&m=136185386310140&w=2
> [2] https://lore.kernel.org/lkml/1479737095.2487.34.camel@linux.vnet.ibm.com/
> [3] https://lore.kernel.org/lkml/1556221605.24945.3.camel@HansenPartnership.com/
> [4] https://lore.kernel.org/linux-integrity/1e41f22b1f11784f1e943f32bf62034d4e054cdb.camel@HansenPartnership.com/
> [5] https://github.com/esnowberg/mokutil/tree/mokvars-v3
> [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
> [7] https://github.com/rhboot/shim
> [8] https://github.com/rhboot/shim/blob/main/BUILDING
>
>
> Eric Snowberg (17):
> KEYS: Create static version of public_key_verify_signature
> integrity: Fix warning about missing prototypes
> integrity: Introduce a Linux keyring called machine
> integrity: Do not allow machine keyring updates following init
> X.509: Parse Basic Constraints for CA
> KEYS: CA link restriction
> integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
> integrity: add new keyring handler for mok keys
> KEYS: Rename get_builtin_and_secondary_restriction
> KEYS: add a reference to machine keyring
> KEYS: Introduce link restriction for machine keys
> KEYS: integrity: change link restriction to trust the machine keyring
> integrity: store reference to machine keyring
> KEYS: link machine trusted keys to secondary_trusted_keys
> efi/mokvar: move up init order
> integrity: Trust MOK keys if MokListTrustedRT found
> integrity: Only use machine keyring when uefi_check_trust_mok_keys is
> true
>
> certs/system_keyring.c | 48 +++++++++++-
> crypto/asymmetric_keys/restrict.c | 43 +++++++++++
> crypto/asymmetric_keys/x509_cert_parser.c | 9 +++
> drivers/firmware/efi/mokvar-table.c | 2 +-
> include/crypto/public_key.h | 25 ++++++
> include/keys/system_keyring.h | 14 ++++
> security/integrity/Kconfig | 12 +++
> security/integrity/Makefile | 1 +
> security/integrity/digsig.c | 23 +++++-
> security/integrity/integrity.h | 17 +++-
> .../platform_certs/keyring_handler.c | 18 ++++-
> .../platform_certs/keyring_handler.h | 5 ++
> security/integrity/platform_certs/load_uefi.c | 4 +-
> .../platform_certs/machine_keyring.c | 77 +++++++++++++++++++
> 14 files changed, 287 insertions(+), 11 deletions(-)
> create mode 100644 security/integrity/platform_certs/machine_keyring.c
>
>
> base-commit: 136057256686de39cc3a07c2e39ef6bc43003ff6
> --
> 2.18.4
>
When I try to apply this:
$ b4 am 20211124044124.998170-8-eric.snowberg@oracle.com
Looking up https://lore.kernel.org/r/20211124044124.998170-8-eric.snowberg%40oracle.com
Analyzing 40 messages in the thread
Checking attestation on all messages, may take a moment...
# ...
$ git am -3 v8_20211123_eric_snowberg_enroll_kernel_keys_thru_mok.mbx
Applying: KEYS: Create static version of public_key_verify_signature
Applying: integrity: Fix warning about missing prototypes
Applying: integrity: Introduce a Linux keyring called machine
Applying: integrity: Do not allow machine keyring updates following init
Applying: X.509: Parse Basic Constraints for CA
Applying: KEYS: CA link restriction
error: sha1 information is lacking or useless (include/crypto/public_key.h).
error: could not build fake ancestor
Patch failed at 0006 KEYS: CA link restriction
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: fix crash when TPM/TEE are built as module
From: Jarkko Sakkinen @ 2022-02-20 23:10 UTC (permalink / raw)
To: Ahmad Fatoum
Cc: Tong Zhang, James Bottomley, Mimi Zohar, David Howells,
James Morris, Serge E. Hallyn, Sumit Garg, linux-integrity,
keyrings, linux-security-module, linux-kernel, Andreas Rammhold
In-Reply-To: <a45010a4-2b86-aa22-d7bd-3c4839356cf1@pengutronix.de>
On Sun, Feb 06, 2022 at 11:36:48AM +0100, Ahmad Fatoum wrote:
> Hello Tong,
>
> On 04.02.22 21:03, Tong Zhang wrote:
> > when TCG_TPM and TEE are built as module, trusted_key_sources will be an
> > empty array, loading it won't do what it is supposed to do and unloading
> > it will cause kernel crash.
>
> Jarkko reported picking up an equivalent fix two months ago:
> https://lkml.kernel.org/keyrings/YadRAWbl2aiapf8l@iki.fi/
>
> But it seems to have never made it to Linus.
Sorry, was not done purposely. I pushed the original fix.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH] KEYS: trusted: fix crash when TPM/TEE are built as module
From: Jarkko Sakkinen @ 2022-02-20 23:05 UTC (permalink / raw)
To: Tong Zhang
Cc: James Bottomley, Mimi Zohar, David Howells, James Morris,
Serge E. Hallyn, Sumit Garg, linux-integrity, keyrings,
linux-security-module, linux-kernel
In-Reply-To: <20220204200342.48665-1-ztong0001@gmail.com>
On Fri, Feb 04, 2022 at 12:03:42PM -0800, Tong Zhang wrote:
> when TCG_TPM and TEE are built as module, trusted_key_sources will be an
> empty array, loading it won't do what it is supposed to do and unloading
> it will cause kernel crash.
>
> To reproduce:
> $ modprobe trusted
> $ modprobe -r trusted
>
> [ 173.749423] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [ 173.755268] Backtrace:
> [ 173.755378] cleanup_trusted [trusted] from sys_delete_module+0x15c/0x22c
> [ 173.755589] sys_delete_module from ret_fast_syscall+0x0/0x1c
>
> To fix this issue, we also need to check CONFIG_TCG_TPM_MODULE and
> CONFIG_TEE_MODULE.
>
> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> Signed-off-by: Tong Zhang <ztong0001@gmail.com>
> ---
> security/keys/trusted-keys/trusted_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index d5c891d8d353..b3a3b2f2d4a4 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
> MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
>
> static const struct trusted_key_source trusted_key_sources[] = {
> -#if defined(CONFIG_TCG_TPM)
> +#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
> { "tpm", &trusted_key_tpm_ops },
> #endif
> -#if defined(CONFIG_TEE)
> +#if defined(CONFIG_TEE) || defined(CONFIG_TEE_MODULE)
> { "tee", &trusted_key_tee_ops },
> #endif
> };
> --
> 2.25.1
>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v8 0/3] integrity: support including firmware ".platform" keys at build time
From: Jarkko Sakkinen @ 2022-02-20 23:02 UTC (permalink / raw)
To: Nayna Jain
Cc: linux-integrity, keyrings, dhowells, zohar, linux-security-module,
linux-kernel, dimitri.ledkov, seth
In-Reply-To: <20220111183647.977037-1-nayna@linux.ibm.com>
On Tue, Jan 11, 2022 at 01:36:44PM -0500, Nayna Jain wrote:
> Some firmware support secure boot by embedding static keys to verify the
> Linux kernel during boot. However, these firmware do not expose an
> interface for the kernel to load firmware keys onto the ".platform"
> keyring, preventing the kernel from verifying the kexec kernel image
> signature.
>
> This patchset exports load_certificate_list() and defines a new function
> load_builtin_platform_cert() to load compiled in certificates onto the
> ".platform" keyring.
>
> Changelog:
>
> v8:
> * Includes Jarkko's feedback on patch description and removed Reported-by
> for Patch 1.
>
> v7:
> * Incldues Jarkko's feedback on patch description for Patch 1 and 3.
>
> v6:
> * Includes Jarkko's feedback:
> * Split Patch 2 into two.
> * Update Patch description.
>
> v5:
> * Renamed load_builtin_platform_cert() to load_platform_certificate_list()
> and config INTEGRITY_PLATFORM_BUILTIN_KEYS to INTEGRITY_PLATFORM_KEYS, as
> suggested by Mimi Zohar.
>
> v4:
> * Split into two patches as per Mimi Zohar and Dimitri John Ledkov
> recommendation.
>
> v3:
> * Included Jarkko's feedback
> ** updated patch description to include approach.
> ** removed extern for function declaration in the .h file.
> * Included load_certificate_list() within #ifdef CONFIG_KEYS condition.
>
> v2:
> * Fixed the error reported by kernel test robot
> * Updated patch description based on Jarkko's feedback.
>
> Nayna Jain (3):
> certs: export load_certificate_list() to be used outside certs/
> integrity: make integrity_keyring_from_id() non-static
> integrity: support including firmware ".platform" keys at build time
>
> certs/Makefile | 5 ++--
> certs/blacklist.c | 1 -
> certs/common.c | 2 +-
> certs/common.h | 9 -------
> certs/system_keyring.c | 1 -
> include/keys/system_keyring.h | 6 +++++
> security/integrity/Kconfig | 10 +++++++
> security/integrity/Makefile | 17 +++++++++++-
> security/integrity/digsig.c | 2 +-
> security/integrity/integrity.h | 6 +++++
> .../integrity/platform_certs/platform_cert.S | 23 ++++++++++++++++
> .../platform_certs/platform_keyring.c | 26 +++++++++++++++++++
> 12 files changed, 92 insertions(+), 16 deletions(-)
> delete mode 100644 certs/common.h
> create mode 100644 security/integrity/platform_certs/platform_cert.S
>
> --
> 2.27.0
To sort out tree conflicts: what if I pick these patches? They look fine
to me now. I can try to fix the possible merge conflicts and you can check
them before I make a PR.
/Jarkko
^ permalink raw reply
* Re: [PATCH v10 0/8] Enroll kernel keys thru MOK
From: Jarkko Sakkinen @ 2022-02-20 19:00 UTC (permalink / raw)
To: Mimi Zohar
Cc: Eric Snowberg, dhowells, dwmw2, ardb, jmorris, serge, nayna,
keescook, torvalds, weiyongjun1, keyrings, linux-kernel,
linux-efi, linux-security-module, James.Bottomley, pjones,
konrad.wilk
In-Reply-To: <78d2c13ad60b5f845cb841d257d1b41290f575c6.camel@linux.ibm.com>
On Wed, Jan 26, 2022 at 05:06:09PM -0500, Mimi Zohar wrote:
> Hi Jarkko,
>
> > > Thank you. I'll pick these soon. Is there any objections?
>
> No objections.
> >
> > Mimi brought up that we need a MAINTAINERS update for this and also
> > .platform.
> >
> > We have these:
> >
> > - KEYS/KEYRINGS
> > - CERTIFICATE HANDLING
> >
> > I would put them under KEYRINGS for now and would not consider further
> > subdivision for the moment.
>
> IMA has dependencies on the platform_certs/ and now on the new .machine
> keyring. Just adding "F: security/integrity/platform_certs/" to the
> KEYS/KEYRINGS record, ignores that dependency. The discussion wouldn't
> even be on the linux-integrity mailing list.
>
> Existing requirement:
> - The keys on the .platform keyring are limited to verifying the kexec
> image.
>
> New requirements based on Eric Snowbergs' patch set:
> - When IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled,
> the MOK keys will not be loaded directly onto the .machine keyring or
> indirectly onto the .secondary_trusted_keys keyring.
>
> - Only when a new IMA Kconfig explicitly allows the keys on the
> .machine keyrings, will the CA keys stored in MOK be loaded onto the
> .machine keyring.
>
> Unfortunately I don't think there is any choice, but to define a new
> MAINTAINERS entry. Perhaps something along the lines of:
>
> KEYS/KEYRINGS_INTEGRITY
> M: Jarkko Sakkinen <jarkko@kernel.org>
> M: Mimi Zohar <zohar@linux.ibm.com>
> L: keyrings@vger.kernel.org
> L: linux-integrity@vger.kernel.org
> F: security/integrity/platform_certs
>
> thanks,
>
> Mimi
This would work for me.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v8 0/5] Enable root to update the blacklist keyring
From: Jarkko Sakkinen @ 2022-02-19 11:42 UTC (permalink / raw)
To: Mickaël Salaün
Cc: David S . Miller, Eric Snowberg, Herbert Xu, James Morris,
Mimi Zohar, Serge E . Hallyn, Tyler Hicks, keyrings, linux-crypto,
linux-integrity, linux-kernel, linux-security-module,
Ahmad Fatoum, Andreas Rammhold, David Howells, David Woodhouse
In-Reply-To: <Yg6o/ARtOIwuBFsW@iki.fi>
On Thu, Feb 17, 2022 at 08:58:57PM +0100, Jarkko Sakkinen wrote:
> On Mon, Jan 31, 2022 at 12:33:51PM +0100, Mickaël Salaün wrote:
> >
> > On 07/01/2022 13:14, Mickaël Salaün wrote:
> > >
> > > On 06/01/2022 20:16, Jarkko Sakkinen wrote:
> > > > On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
> > > > > On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
> > > > > >
> > > > > > On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > > > > > > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> > > > > > > > Hi Jarkko,
> > > > > > > >
> > > > > > > > Since everyone seems OK with this and had plenty of
> > > > > > > > time to complain, could
> > > > > > > > you please take this patch series in your tree? It still applies on
> > > > > > > > v5.16-rc5 and it is really important to us. Please
> > > > > > > > let me know if you need
> > > > > > > > something more.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Mickaël
> > > > > > >
> > > > > > > I'm off-work up until end of the year, i.e. I will
> > > > > > > address only important
> > > > > > > bug fixes and v5.16 up until that.
> > > > > > >
> > > > > > > If any of the patches is yet missing my ack, feel free to
> > > > > > >
> > > > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > >
> > > > > > Thanks Jarkko. Can you please take it into your tree?
> > > > >
> > > > > I can yes, as I need to anyway do a revised PR for v5.17, as one commit
> > > > > in my first trial had a truncated fixes tag.
> > > >
> > > > Please check:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
> > > >
> > > > /Jarkko
> > >
> > > Great, thanks!
> >
> > Hi Jarkko,
> >
> > I noticed your commits https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=3ec9c3a0531ac868422be3b12fc17310ed8c07dc
> > are no more referenced in your tree. Is there an issue?
>
> This must be some sort of mistake I've made. I'll re-apply the patches.
> Sorry about this.
OK now the patches are in and will be included to the next PR. I fixed
merge conflicts caused by 5cca36069d4c ("certs: refactor file cleaning")
in "certs: Check that builtin blacklist hashes are valid" so please
sanity check that it is good.
BR, Jarkko
^ permalink raw reply
* Re: [PATCH v10 16/27] ima: Implement ima_free_policy_rules() for freeing of an ima_namespace
From: Mimi Zohar @ 2022-02-18 20:04 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <55c0c300-3fe4-f610-0b78-adc5593a70a0@linux.ibm.com>
On Fri, 2022-02-18 at 14:38 -0500, Stefan Berger wrote:
> On 2/18/22 12:09, Mimi Zohar wrote:
> > On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> >> Implement ima_free_policy_rules() that is needed when an ima_namespace
> >> is freed.
ima_free_policy_rules() isn't free all the rules, just the custom
policy rules. I would update the patch description as:
Implement ima_free_policy_rules() to free the custom policy rules, when
...
Otherwise,
Reviewd-by: Mimi Zohar <zohar@linux.ibm.com>
> >>
> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> >>
> >> ---
> >> v10:
> >> - Not calling ima_delete_rules() anymore
> >> - Move access check from ima_delete_rules into very last patch
> >>
> >> v9:
> >> - Only reset temp_ima_appraise when using init_ima_ns.
> >> ---
> >> security/integrity/ima/ima.h | 1 +
> >> security/integrity/ima/ima_policy.c | 14 ++++++++++++++
> >> 2 files changed, 15 insertions(+)
> >>
> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> >> index aea8fb8d2854..8c757223d549 100644
> >> --- a/security/integrity/ima/ima.h
> >> +++ b/security/integrity/ima/ima.h
> >> @@ -329,6 +329,7 @@ void ima_update_policy_flags(struct ima_namespace *ns);
> >> ssize_t ima_parse_add_rule(struct ima_namespace *ns, char *rule);
> >> void ima_delete_rules(struct ima_namespace *ns);
> >> int ima_check_policy(struct ima_namespace *ns);
> >> +void ima_free_policy_rules(struct ima_namespace *ns);
> >> void *ima_policy_start(struct seq_file *m, loff_t *pos);
> >> void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> >> void ima_policy_stop(struct seq_file *m, void *v);
> >> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> >> index 2dcc5a8c585a..fe3dce8fb939 100644
> >> --- a/security/integrity/ima/ima_policy.c
> >> +++ b/security/integrity/ima/ima_policy.c
> >> @@ -1889,6 +1889,20 @@ void ima_delete_rules(struct ima_namespace *ns)
> >> }
> >> }
> >>
> >> +/**
> >> + * ima_free_policy_rules - free all policy rules
> >> + * @ns: IMA namespace that has the policy
> >> + */
> >> +void ima_free_policy_rules(struct ima_namespace *ns)
> >> +{
> >> + struct ima_rule_entry *entry, *tmp;
> >> +
> >> + list_for_each_entry_safe(entry, tmp, &ns->ima_policy_rules, list) {
> >> + list_del(&entry->list);
> >> + ima_free_rule(entry);
> >> + }
> >> +}
> >> +
> > The first time a policy is loaded, the policy rules pivot
> > from ima_default_rules to the custom rules. When this happens, the
> > architecture specific policy rules are freed. Here too, if a custom
> > policy isn't already loaded, the architecture specific rules stored as
> > an array need to be freed. Refer to the comment in
> > ima_update_policy().
>
> Right. So here's how it's done (before arch_policy_entry was moved into
> ima_namespace).
>
> /*
> * IMA architecture specific policy rules are specified
> * as strings and converted to an array of ima_entry_rules
> * on boot. After loading a custom policy, free the
> * architecture specific rules stored as an array.
> */
> kfree(arch_policy_entry);
>
>
> So, I now added kfree(ns->arch_policy_entry).
Yes, that is fine.
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v10 17/27] ima: Add functions for creating and freeing of an ima_namespace
From: Mimi Zohar @ 2022-02-18 19:49 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <20220201203735.164593-18-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Implement create_ima_ns() to create an empty ima_namespace. Defer its
> initialization to a later point outside this function. Implement
> free_ima_ns() to free it.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
>
> ---
> v9:
> - Set user_ns->ims_ns = NULL in free_ima_ns()
> - Refactored create_ima_ns() to defer initialization
> - Removed pr_debug functions
> ---
> include/linux/ima.h | 13 ++++++
> security/integrity/ima/Makefile | 1 +
> security/integrity/ima/ima.h | 15 +++++++
> security/integrity/ima/ima_init_ima_ns.c | 2 +-
> security/integrity/ima/ima_ns.c | 53 ++++++++++++++++++++++++
> 5 files changed, 83 insertions(+), 1 deletion(-)
> create mode 100644 security/integrity/ima/ima_ns.c
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 0cf2a80c8b35..964a08702573 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -220,4 +220,17 @@ static inline bool ima_appraise_signature(enum kernel_read_file_id func)
> return false;
> }
> #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
> +
> +#ifdef CONFIG_IMA_NS
> +
> +void free_ima_ns(struct user_namespace *ns);
> +
> +#else
> +
> +static inline void free_ima_ns(struct user_namespace *user_ns)
> +{
> +}
> +
> +#endif /* CONFIG_IMA_NS */
> +
> #endif /* _LINUX_IMA_H */
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index f8a5e5f3975d..b86a35fbed60 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -14,6 +14,7 @@ ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
> ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
> ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
> +ima-$(CONFIG_IMA_NS) += ima_ns.o
>
> ifeq ($(CONFIG_EFI),y)
> ima-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_efi.o
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 8c757223d549..751e936a6311 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -167,6 +167,7 @@ extern bool ima_canonical_fmt;
> int ima_init(void);
> int ima_fs_init(void);
> int ima_ns_init(void);
> +int ima_init_namespace(struct ima_namespace *ns);
> int ima_add_template_entry(struct ima_namespace *ns,
> struct ima_template_entry *entry, int violation,
> const char *op, struct inode *inode,
> @@ -502,4 +503,18 @@ static inline struct ima_namespace
> return NULL;
> }
>
> +#ifdef CONFIG_IMA_NS
> +
> +struct ima_namespace *create_ima_ns(void);
> +
> +#else
> +
> +static inline struct ima_namespace *create_ima_ns(void)
> +{
> + WARN(1, "Cannot create an IMA namespace\n");
> + return ERR_PTR(-EFAULT);
> +}
> +
> +#endif /* CONFIG_IMA_NS */
> +
> #endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
> index 1cba545ae477..166dab4a3126 100644
> --- a/security/integrity/ima/ima_init_ima_ns.c
> +++ b/security/integrity/ima/ima_init_ima_ns.c
> @@ -8,7 +8,7 @@
>
> #include "ima.h"
>
> -static int ima_init_namespace(struct ima_namespace *ns)
> +int ima_init_namespace(struct ima_namespace *ns)
> {
> int rc;
>
> diff --git a/security/integrity/ima/ima_ns.c b/security/integrity/ima/ima_ns.c
> new file mode 100644
> index 000000000000..b3b81a1e313e
> --- /dev/null
> +++ b/security/integrity/ima/ima_ns.c
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2016-2021 IBM Corporation
> + * Author:
> + * Yuqiong Sun <suny@us.ibm.com>
> + * Stefan Berger <stefanb@linux.vnet.ibm.com>
> + */
> +
> +#include <linux/ima.h>
> +
> +#include "ima.h"
> +
> +static struct kmem_cache *imans_cachep;
> +
> +struct ima_namespace *create_ima_ns(void)
> +{
> + struct ima_namespace *ns;
> +
> + ns = kmem_cache_zalloc(imans_cachep, GFP_KERNEL);
> + if (!ns)
> + return ERR_PTR(-ENOMEM);
> +
> + return ns;
> +}
> +
> +/* destroy_ima_ns() must only be called after ima_init_namespace() was called */
> +static void destroy_ima_ns(struct ima_namespace *ns)
> +{
> + unregister_blocking_lsm_notifier(&ns->ima_lsm_policy_notifier);
> + kfree(ns->arch_policy_entry);
Oh! Here's the missing free of the architecture specific rules
(comment on 16/27). Even if these rules have already been freed after
a custom policy is loaded, I guess it is safe to free them again.
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> + ima_free_policy_rules(ns);
> +}
> +
> +void free_ima_ns(struct user_namespace *user_ns)
> +{
> + struct ima_namespace *ns = user_ns->ima_ns;
> +
> + if (!ns || WARN_ON(ns == &init_ima_ns))
> + return;
> +
> + destroy_ima_ns(ns);
> +
> + kmem_cache_free(imans_cachep, ns);
> +
> + user_ns->ima_ns = NULL;
> +}
> +
> +static int __init imans_cache_init(void)
> +{
> + imans_cachep = KMEM_CACHE(ima_namespace, SLAB_PANIC);
> + return 0;
> +}
> +subsys_initcall(imans_cache_init)
^ permalink raw reply
* Re: [PATCH v10 16/27] ima: Implement ima_free_policy_rules() for freeing of an ima_namespace
From: Stefan Berger @ 2022-02-18 19:38 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <ce1fbc8baf5359b698bf4420e602cc3a5a2a1f44.camel@linux.ibm.com>
On 2/18/22 12:09, Mimi Zohar wrote:
> On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
>> Implement ima_free_policy_rules() that is needed when an ima_namespace
>> is freed.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>
>> ---
>> v10:
>> - Not calling ima_delete_rules() anymore
>> - Move access check from ima_delete_rules into very last patch
>>
>> v9:
>> - Only reset temp_ima_appraise when using init_ima_ns.
>> ---
>> security/integrity/ima/ima.h | 1 +
>> security/integrity/ima/ima_policy.c | 14 ++++++++++++++
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index aea8fb8d2854..8c757223d549 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -329,6 +329,7 @@ void ima_update_policy_flags(struct ima_namespace *ns);
>> ssize_t ima_parse_add_rule(struct ima_namespace *ns, char *rule);
>> void ima_delete_rules(struct ima_namespace *ns);
>> int ima_check_policy(struct ima_namespace *ns);
>> +void ima_free_policy_rules(struct ima_namespace *ns);
>> void *ima_policy_start(struct seq_file *m, loff_t *pos);
>> void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
>> void ima_policy_stop(struct seq_file *m, void *v);
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index 2dcc5a8c585a..fe3dce8fb939 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -1889,6 +1889,20 @@ void ima_delete_rules(struct ima_namespace *ns)
>> }
>> }
>>
>> +/**
>> + * ima_free_policy_rules - free all policy rules
>> + * @ns: IMA namespace that has the policy
>> + */
>> +void ima_free_policy_rules(struct ima_namespace *ns)
>> +{
>> + struct ima_rule_entry *entry, *tmp;
>> +
>> + list_for_each_entry_safe(entry, tmp, &ns->ima_policy_rules, list) {
>> + list_del(&entry->list);
>> + ima_free_rule(entry);
>> + }
>> +}
>> +
> The first time a policy is loaded, the policy rules pivot
> from ima_default_rules to the custom rules. When this happens, the
> architecture specific policy rules are freed. Here too, if a custom
> policy isn't already loaded, the architecture specific rules stored as
> an array need to be freed. Refer to the comment in
> ima_update_policy().
Right. So here's how it's done (before arch_policy_entry was moved into
ima_namespace).
/*
* IMA architecture specific policy rules are specified
* as strings and converted to an array of ima_entry_rules
* on boot. After loading a custom policy, free the
* architecture specific rules stored as an array.
*/
kfree(arch_policy_entry);
So, I now added kfree(ns->arch_policy_entry).
Thanks.
Stefan
>
>> #define __ima_hook_stringify(func, str) (#func),
>>
>> const char *const func_tokens[] = {
^ permalink raw reply
* Új hirdetési kapcsolatfelvétel
From: WordPress @ 2022-02-18 18:25 UTC (permalink / raw)
To: linux-security-module
❤️ You have unread messages from Donna (2)! Click Here: https://clck.ru/bm7zq?lgr ❤️ érdeklődő a következő üzentet küldte:
pmidkb
Telefonszáma: 908488797740
Email címe: linux-security-module@vger.kernel.org
Település ahonnan érdeklődik: 93021
^ permalink raw reply
* Re: [PATCH v10 16/27] ima: Implement ima_free_policy_rules() for freeing of an ima_namespace
From: Mimi Zohar @ 2022-02-18 17:09 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <20220201203735.164593-17-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Implement ima_free_policy_rules() that is needed when an ima_namespace
> is freed.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>
> ---
> v10:
> - Not calling ima_delete_rules() anymore
> - Move access check from ima_delete_rules into very last patch
>
> v9:
> - Only reset temp_ima_appraise when using init_ima_ns.
> ---
> security/integrity/ima/ima.h | 1 +
> security/integrity/ima/ima_policy.c | 14 ++++++++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index aea8fb8d2854..8c757223d549 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -329,6 +329,7 @@ void ima_update_policy_flags(struct ima_namespace *ns);
> ssize_t ima_parse_add_rule(struct ima_namespace *ns, char *rule);
> void ima_delete_rules(struct ima_namespace *ns);
> int ima_check_policy(struct ima_namespace *ns);
> +void ima_free_policy_rules(struct ima_namespace *ns);
> void *ima_policy_start(struct seq_file *m, loff_t *pos);
> void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> void ima_policy_stop(struct seq_file *m, void *v);
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 2dcc5a8c585a..fe3dce8fb939 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1889,6 +1889,20 @@ void ima_delete_rules(struct ima_namespace *ns)
> }
> }
>
> +/**
> + * ima_free_policy_rules - free all policy rules
> + * @ns: IMA namespace that has the policy
> + */
> +void ima_free_policy_rules(struct ima_namespace *ns)
> +{
> + struct ima_rule_entry *entry, *tmp;
> +
> + list_for_each_entry_safe(entry, tmp, &ns->ima_policy_rules, list) {
> + list_del(&entry->list);
> + ima_free_rule(entry);
> + }
> +}
> +
The first time a policy is loaded, the policy rules pivot
from ima_default_rules to the custom rules. When this happens, the
architecture specific policy rules are freed. Here too, if a custom
policy isn't already loaded, the architecture specific rules stored as
an array need to be freed. Refer to the comment in
ima_update_policy().
> #define __ima_hook_stringify(func, str) (#func),
>
> const char *const func_tokens[] = {
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v10 15/27] ima: Implement hierarchical processing of file accesses
From: Mimi Zohar @ 2022-02-18 16:27 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <20220201203735.164593-16-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Implement hierarchical processing of file accesses in IMA namespaces by
> walking the list of user namespaces towards the root. This way file
> accesses can be audited in an IMA namespace and also be evaluated against
> the IMA policies of parent IMA namespaces.
>
> Pass the user_namespace into process_measurement since we will be walking
> the hierarchy of user_namespaces towards the init_user_ns and we can easily
> derive the ima_namespace from the user_namespace.
>
> __process_measurement() returns either 0 or -EACCES. For hierarchical
> processing remember the -EACCES returned by this function but continue
> to the parent user namespace. At the end either return 0 or -EACCES
> if an error occurred in one of the IMA namespaces.
>
> Currently the ima_ns pointer of the user_namespace is always NULL except
> at the init_user_ns, so test ima_ns for NULL pointer and skip the call to
> __process_measurement() if it is NULL. Once IMA namespacing is fully
> enabled, the pointer may still be NULL due to late initialization of the
> IMA namespace.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>
> ---
>
> v10:
> - Fixed compilation issue
>
> v9:
> - Switch callers to pass user_namespace rather than ima_namespace with
> potential NULL pointer
> - Add default case to switch statement and warn if this happens
> - Implement ima_ns_from_user_ns() in this patch now
> ---
> security/integrity/ima/ima.h | 8 ++++
> security/integrity/ima/ima_main.c | 76 +++++++++++++++++++++++--------
> 2 files changed, 65 insertions(+), 19 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 0057b1fd6c18..aea8fb8d2854 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -493,4 +493,12 @@ struct user_namespace *ima_user_ns_from_file(const struct file *filp)
> return file_inode(filp)->i_sb->s_user_ns;
> }
>
> +static inline struct ima_namespace
> +*ima_ns_from_user_ns(struct user_namespace *user_ns)
> +{
> + if (user_ns == &init_user_ns)
> + return &init_ima_ns;
> + return NULL;
> +}
> +
> #endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index ae0e9b14554a..917504319e7f 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -196,10 +196,10 @@ void ima_file_free(struct file *file)
> ima_check_last_writer(iint, inode, file);
> }
>
> -static int process_measurement(struct ima_namespace *ns,
> - struct file *file, const struct cred *cred,
> - u32 secid, char *buf, loff_t size, int mask,
> - enum ima_hooks func)
> +static int __process_measurement(struct ima_namespace *ns,
> + struct file *file, const struct cred *cred,
> + u32 secid, char *buf, loff_t size, int mask,
> + enum ima_hooks func)
> {
> struct inode *inode = file_inode(file);
> struct integrity_iint_cache *iint = NULL;
> @@ -391,6 +391,41 @@ static int process_measurement(struct ima_namespace *ns,
> return 0;
> }
>
> +static int process_measurement(struct user_namespace *user_ns,
> + struct file *file, const struct cred *cred,
> + u32 secid, char *buf, loff_t size, int mask,
> + enum ima_hooks func)
> +{
> + struct ima_namespace *ns;
> + int ret = 0;
> +
> + while (user_ns) {
> + ns = ima_ns_from_user_ns(user_ns);
> + if (ns) {
> + int rc;
> +
> + rc = __process_measurement(ns, file, cred, secid, buf,
> + size, mask, func);
> + switch (rc) {
> + case 0:
> + break;
> + case -EACCES:
> + /* return this error at the end but continue */
> + ret = -EACCES;
> + break;
> + default:
> + /* should not happen */
> + ret = -EACCES;
> + WARN_ON_ONCE(true);
> + }
> + }
> +
> + user_ns = user_ns->parent;
> + };
> +
> + return ret;
> +}
> +
Very nice and concise!
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v10 14/27] userns: Add pointer to ima_namespace to user_namespace
From: Mimi Zohar @ 2022-02-18 16:26 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <20220201203735.164593-15-stefanb@linux.ibm.com>
Hi Stefan,
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Add a pointer to ima_namespace to the user_namespace and initialize
> the init_user_ns with a pointer to init_ima_ns.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
The patch description explains 'what' is being done, but not 'why'.
Please add a sentence before what you have providing the motivation.
Otherwise,
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
--
thanks,
Mimi
^ permalink raw reply
* RE: [PATCH v2 0/6] bpf-lsm: Extend interoperability with IMA
From: Roberto Sassu @ 2022-02-18 15:01 UTC (permalink / raw)
To: zohar@linux.ibm.com, shuah@kernel.org, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, kpsingh@kernel.org,
revest@chromium.org
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kselftest@vger.kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20220215124042.186506-1-roberto.sassu@huawei.com>
> From: Roberto Sassu
> Sent: Tuesday, February 15, 2022 1:41 PM
> Extend the interoperability with IMA, to give wider flexibility for the
> implementation of integrity-focused LSMs based on eBPF.
>
> Patch 1 fixes some style issues.
>
> Patches 2-4 gives the ability to eBPF-based LSMs to take advantage of the
> measurement capability of IMA without needing to setup a policy in IMA
> (those LSMs might implement the policy capability themselves).
>
> Patches 5-6 allows eBPF-based LSMs to evaluate files read by the kernel.
Hi everyone
I published the new DIGLIM eBPF, that takes advantage of
the new features introduced with this patch set:
https://github.com/robertosassu/diglim-ebpf
the eBPF program is in ebpf/diglim_kern.c
If you could have a look and give me some comments
or suggestions, it would be very appreciated!
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Zhong Ronghua
> Changelog
>
> v1:
> - Modify ima_file_hash() only and allow the usage of the function with the
> modified behavior by eBPF-based LSMs through the new function
> bpf_ima_file_hash() (suggested by Mimi)
> - Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash()
> and bpf_ima_file_hash() can be called inside the implementation of
> eBPF-based LSMs for this hook
>
> Roberto Sassu (6):
> ima: Fix documentation-related warnings in ima_main.c
> ima: Always return a file measurement in ima_file_hash()
> bpf-lsm: Introduce new helper bpf_ima_file_hash()
> selftests/bpf: Add test for bpf_ima_file_hash()
> bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable
> selftests/bpf: Add test for bpf_lsm_kernel_read_file()
>
> include/uapi/linux/bpf.h | 11 +++++
> kernel/bpf/bpf_lsm.c | 21 +++++++++
> security/integrity/ima/ima_main.c | 47 ++++++++++++-------
> tools/include/uapi/linux/bpf.h | 11 +++++
> tools/testing/selftests/bpf/ima_setup.sh | 2 +
> .../selftests/bpf/prog_tests/test_ima.c | 30 ++++++++++--
> tools/testing/selftests/bpf/progs/ima.c | 34 ++++++++++++--
> 7 files changed, 132 insertions(+), 24 deletions(-)
>
> --
> 2.32.0
^ permalink raw reply
* Re: [PATCH] security: declare member holding string literal const
From: Casey Schaufler @ 2022-02-17 22:50 UTC (permalink / raw)
To: Christian Göttsche, selinux
Cc: James Morris, Serge E. Hallyn, Nathan Chancellor,
Nick Desaulniers, Paul Moore, Xin Long, David S. Miller,
Ondrej Mosnacek, Mickaël Salaün, Todd Kjos,
Olga Kornievskaia, linux-kernel, linux-security-module, llvm,
Casey Schaufler
In-Reply-To: <20220217141858.71281-1-cgzones@googlemail.com>
On 2/17/2022 6:18 AM, Christian Göttsche wrote:
> The struct security_hook_list member lsm is assigned in
> security_add_hooks() with string literals passed from the individual
> security modules. Declare the function parameter and the struct member
> const to signal their immutability.
>
> Reported by Clang [-Wwrite-strings]:
>
> security/selinux/hooks.c:7388:63: error: passing 'const char [8]' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), selinux);
> ^~~~~~~~~
> ./include/linux/lsm_hooks.h:1629:11: note: passing argument to parameter 'lsm' here
> char *lsm);
> ^
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 4 ++--
> security/security.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 419b5febc3ca..47cdf3fbecef 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1595,7 +1595,7 @@ struct security_hook_list {
> struct hlist_node list;
> struct hlist_head *head;
> union security_list_options hook;
> - char *lsm;
> + const char *lsm;
> } __randomize_layout;
>
> /*
> @@ -1630,7 +1630,7 @@ extern struct security_hook_heads security_hook_heads;
> extern char *lsm_names;
>
> extern void security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm);
> + const char *lsm);
>
> #define LSM_FLAG_LEGACY_MAJOR BIT(0)
> #define LSM_FLAG_EXCLUSIVE BIT(1)
> diff --git a/security/security.c b/security/security.c
> index 9663ffcca4b0..a48eb3badfdd 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -478,7 +478,7 @@ static int lsm_append(const char *new, char **result)
> * Each LSM has to register its hooks with the infrastructure.
> */
> void __init security_add_hooks(struct security_hook_list *hooks, int count,
> - char *lsm)
> + const char *lsm)
> {
> int i;
>
^ permalink raw reply
* Re: [RFC PATCH] mm: create security context for memfd_secret inodes
From: Paul Moore @ 2022-02-17 22:32 UTC (permalink / raw)
To: Christian Göttsche
Cc: SElinux list, James Morris, Serge E. Hallyn,
linux-security-module, Stephen Smalley, Eric Paris, Andrew Morton,
linux-mm, linux-kernel
In-Reply-To: <CAJ2a_DeAUcGTGm_fk8viVbeFXr6FLrJ-oLw-abwFND6Kv0u0gQ@mail.gmail.com>
On Thu, Feb 17, 2022 at 9:24 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
> On Thu, 27 Jan 2022 at 00:01, Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jan 25, 2022 at 9:33 AM Christian Göttsche
> > <cgzones@googlemail.com> wrote:
> > >
> > > Create a security context for the inodes created by memfd_secret(2) via
> > > the LSM hook inode_init_security_anon to allow a fine grained control.
> > > As secret memory areas can affect hibernation and have a global shared
> > > limit access control might be desirable.
> > >
> > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > > ---
> > > An alternative way of checking memfd_secret(2) is to create a new LSM
> > > hook and e.g. for SELinux check via a new process class permission.
> > > ---
> > > mm/secretmem.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> >
> > This seems reasonable to me, and I like the idea of labeling the anon
> > inode as opposed to creating a new set of LSM hooks. If we want to
> > apply access control policy to the memfd_secret() fds we are going to
> > need to attach some sort of LSM state to the inode, we might as well
> > use the mechanism we already have instead of inventing another one.
>
> Any further comments (on design or implementation)?
>
> Should I resend a non-rfc?
I personally would really like to see a selinux-testsuite for this so
that we can verify it works not just now but in the future too. I
think having a test would also help demonstrate the usefulness of the
additional LSM controls.
> One naming question:
> Should the anonymous inode class be named "[secretmem]", like
> "[userfaultfd]", or "[secret_mem]" similar to "[io_uring]"?
The pr_fmt() string in mm/secretmem.c uses "secretmem" so I would
suggest sticking with "[secretmem]", although that is question best
answered by the secretmem maintainer.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH] security: declare member holding string literal const
From: Paul Moore @ 2022-02-17 22:27 UTC (permalink / raw)
To: Christian Göttsche
Cc: selinux, James Morris, Serge E. Hallyn, Nathan Chancellor,
Nick Desaulniers, Casey Schaufler, Xin Long, David S. Miller,
Ondrej Mosnacek, Mickaël Salaün, Todd Kjos,
Olga Kornievskaia, linux-kernel, linux-security-module, llvm
In-Reply-To: <20220217141858.71281-1-cgzones@googlemail.com>
On Thu, Feb 17, 2022 at 9:19 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> The struct security_hook_list member lsm is assigned in
> security_add_hooks() with string literals passed from the individual
> security modules. Declare the function parameter and the struct member
> const to signal their immutability.
>
> Reported by Clang [-Wwrite-strings]:
>
> security/selinux/hooks.c:7388:63: error: passing 'const char [8]' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), selinux);
> ^~~~~~~~~
> ./include/linux/lsm_hooks.h:1629:11: note: passing argument to parameter 'lsm' here
> char *lsm);
> ^
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> include/linux/lsm_hooks.h | 4 ++--
> security/security.c | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
Thanks Christian.
Reviewed-by: Paul Moore <paul@paul-moore.com>
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v10 13/27] ima: Only accept AUDIT rules for non-init_ima_ns namespaces for now
From: Mimi Zohar @ 2022-02-17 21:32 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris, Christian Brauner
In-Reply-To: <20220201203735.164593-14-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
> Only accept AUDIT rules for non-init_ima_ns namespaces rejecting all rules
> that require support for measuring, appraisal, and hashing.
It's probably obvious, but adding the words "for now" somewhere in the
above line makes it clear this is temporary.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v10 11/27] ima: Move ima_lsm_policy_notifier into ima_namespace
From: Mimi Zohar @ 2022-02-17 21:24 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <95a9eb64-654b-8d34-12c7-1dd7666e3420@linux.ibm.com>
On Thu, 2022-02-17 at 15:59 -0500, Stefan Berger wrote:
> >
> >> Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
> >> namespace can now register its own LSM policy change notifier callback.
> >> The policy change notifier for the init_ima_ns still remains in init_ima()
> >> and therefore handle the registration of the callback for all other
> >> namespaces in init_ima_namespace().
> >>
> >> Suppress the kernel warning 'rule for LSM <label> is undefined` for all
> >> other IMA namespaces than the init_ima_ns.
> > Instead of ignoring the warnings totally, perhaps use either the
> > "ratelimited" or "once" function options for non init_ima_ns. It would
> > be nice if these functions could be namespace aware, so that each
> > affected IMA namespace would contain at least one warning.
>
> The problem is that any user can now repeatedly create user namespaces
> and with that IMA namespaces and cause the kernel log to fill up with
> these messages and also flood the audit log -- I guess one could
> describe it as an unwanted side-effect. I am afraid that for as long as
> the kernel log is not namespaced it's probably best to just turn them
> off for non-init_ima_ns.
There are functions - pr_warn_once() or pr_warn_ratelimited() - that
limit the number of kernel messages. In addition to the number of
potential kernel messages, there's also the issue of being able to
differentiate between init_ima_ns and other IMA namespaces. I think
that is more of a concern than rate limiting.
--
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v10 11/27] ima: Move ima_lsm_policy_notifier into ima_namespace
From: Stefan Berger @ 2022-02-17 20:59 UTC (permalink / raw)
To: Mimi Zohar, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <62f946ec160296b6b20bee98986b2bafb8427718.camel@linux.ibm.com>
On 2/17/22 15:30, Mimi Zohar wrote:
> On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
>
> The builtin IMA policy rules are broad and may be constrained by
> loading a custom policy, which could be defined in terms of LSM labels.
> When an LSM policy is loaded, existing LSM labels might be affected or
> even removed. In either case, IMA policy rules based on LSM
> labels, need to reflect these changes. If an LSM label is removed,
> instead of deleting the IMA policy rule based on the LSM label, the IMA
> policy rule is made inactive.
Ok, so I take this paragraph for the patch description.
>
>> Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
>> namespace can now register its own LSM policy change notifier callback.
>> The policy change notifier for the init_ima_ns still remains in init_ima()
>> and therefore handle the registration of the callback for all other
>> namespaces in init_ima_namespace().
>>
>> Suppress the kernel warning 'rule for LSM <label> is undefined` for all
>> other IMA namespaces than the init_ima_ns.
> Instead of ignoring the warnings totally, perhaps use either the
> "ratelimited" or "once" function options for non init_ima_ns. It would
> be nice if these functions could be namespace aware, so that each
> affected IMA namespace would contain at least one warning.
The problem is that any user can now repeatedly create user namespaces
and with that IMA namespaces and cause the kernel log to fill up with
these messages and also flood the audit log -- I guess one could
describe it as an unwanted side-effect. I am afraid that for as long as
the kernel log is not namespaced it's probably best to just turn them
off for non-init_ima_ns.
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Thanks.
>
>> ---
>> v10:
>> - Only call pr_warn('rule for LSM <label> is undefined`) for init_ima_ns
>> ---
>> security/integrity/ima/ima.h | 2 ++
>> security/integrity/ima/ima_init_ima_ns.c | 14 ++++++++++++++
>> security/integrity/ima/ima_main.c | 6 +-----
>> security/integrity/ima/ima_policy.c | 16 ++++++++++------
>> 4 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 94c6e3a4d666..fb6bd054d899 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -144,6 +144,8 @@ struct ima_namespace {
>> int valid_policy;
>>
>> struct dentry *ima_policy;
>> +
>> + struct notifier_block ima_lsm_policy_notifier;
>> } __randomize_layout;
>> extern struct ima_namespace init_ima_ns;
>>
>> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
>> index 425eed1c6838..1cba545ae477 100644
>> --- a/security/integrity/ima/ima_init_ima_ns.c
>> +++ b/security/integrity/ima/ima_init_ima_ns.c
>> @@ -10,6 +10,8 @@
>>
>> static int ima_init_namespace(struct ima_namespace *ns)
>> {
>> + int rc;
>> +
> There's no right or wrong, but the newer IMA code uses 'ret'.
I don't mind to change it. I will take your Reviewed-by, though :-)
>
>> INIT_LIST_HEAD(&ns->ima_default_rules);
>> INIT_LIST_HEAD(&ns->ima_policy_rules);
>> INIT_LIST_HEAD(&ns->ima_temp_rules);
^ permalink raw reply
* Re: [PATCH v10 11/27] ima: Move ima_lsm_policy_notifier into ima_namespace
From: Mimi Zohar @ 2022-02-17 20:30 UTC (permalink / raw)
To: Stefan Berger, linux-integrity
Cc: serge, christian.brauner, containers, dmitry.kasatkin, ebiederm,
krzysztof.struczynski, roberto.sassu, mpeters, lhinds, lsturman,
puiterwi, jejb, jamjoom, linux-kernel, paul, rgb,
linux-security-module, jmorris
In-Reply-To: <20220201203735.164593-12-stefanb@linux.ibm.com>
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
The builtin IMA policy rules are broad and may be constrained by
loading a custom policy, which could be defined in terms of LSM labels.
When an LSM policy is loaded, existing LSM labels might be affected or
even removed. In either case, IMA policy rules based on LSM
labels, need to reflect these changes. If an LSM label is removed,
instead of deleting the IMA policy rule based on the LSM label, the IMA
policy rule is made inactive.
> Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
> namespace can now register its own LSM policy change notifier callback.
> The policy change notifier for the init_ima_ns still remains in init_ima()
> and therefore handle the registration of the callback for all other
> namespaces in init_ima_namespace().
>
> Suppress the kernel warning 'rule for LSM <label> is undefined` for all
> other IMA namespaces than the init_ima_ns.
Instead of ignoring the warnings totally, perhaps use either the
"ratelimited" or "once" function options for non init_ima_ns. It would
be nice if these functions could be namespace aware, so that each
affected IMA namespace would contain at least one warning.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>
> ---
> v10:
> - Only call pr_warn('rule for LSM <label> is undefined`) for init_ima_ns
> ---
> security/integrity/ima/ima.h | 2 ++
> security/integrity/ima/ima_init_ima_ns.c | 14 ++++++++++++++
> security/integrity/ima/ima_main.c | 6 +-----
> security/integrity/ima/ima_policy.c | 16 ++++++++++------
> 4 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 94c6e3a4d666..fb6bd054d899 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -144,6 +144,8 @@ struct ima_namespace {
> int valid_policy;
>
> struct dentry *ima_policy;
> +
> + struct notifier_block ima_lsm_policy_notifier;
> } __randomize_layout;
> extern struct ima_namespace init_ima_ns;
>
> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
> index 425eed1c6838..1cba545ae477 100644
> --- a/security/integrity/ima/ima_init_ima_ns.c
> +++ b/security/integrity/ima/ima_init_ima_ns.c
> @@ -10,6 +10,8 @@
>
> static int ima_init_namespace(struct ima_namespace *ns)
> {
> + int rc;
> +
There's no right or wrong, but the newer IMA code uses 'ret'.
> INIT_LIST_HEAD(&ns->ima_default_rules);
> INIT_LIST_HEAD(&ns->ima_policy_rules);
> INIT_LIST_HEAD(&ns->ima_temp_rules);
^ permalink raw reply
* Re: [PATCH v8 0/5] Enable root to update the blacklist keyring
From: Jarkko Sakkinen @ 2022-02-17 19:58 UTC (permalink / raw)
To: Mickaël Salaün
Cc: David S . Miller, Eric Snowberg, Herbert Xu, James Morris,
Mimi Zohar, Serge E . Hallyn, Tyler Hicks, keyrings, linux-crypto,
linux-integrity, linux-kernel, linux-security-module,
Ahmad Fatoum, Andreas Rammhold, David Howells, David Woodhouse
In-Reply-To: <e4707df2-ecc2-0471-87fc-c54e774fe315@digikod.net>
On Mon, Jan 31, 2022 at 12:33:51PM +0100, Mickaël Salaün wrote:
>
> On 07/01/2022 13:14, Mickaël Salaün wrote:
> >
> > On 06/01/2022 20:16, Jarkko Sakkinen wrote:
> > > On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
> > > > >
> > > > > On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > > > > > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> > > > > > > Hi Jarkko,
> > > > > > >
> > > > > > > Since everyone seems OK with this and had plenty of
> > > > > > > time to complain, could
> > > > > > > you please take this patch series in your tree? It still applies on
> > > > > > > v5.16-rc5 and it is really important to us. Please
> > > > > > > let me know if you need
> > > > > > > something more.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Mickaël
> > > > > >
> > > > > > I'm off-work up until end of the year, i.e. I will
> > > > > > address only important
> > > > > > bug fixes and v5.16 up until that.
> > > > > >
> > > > > > If any of the patches is yet missing my ack, feel free to
> > > > > >
> > > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > >
> > > > > Thanks Jarkko. Can you please take it into your tree?
> > > >
> > > > I can yes, as I need to anyway do a revised PR for v5.17, as one commit
> > > > in my first trial had a truncated fixes tag.
> > >
> > > Please check:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
> > >
> > > /Jarkko
> >
> > Great, thanks!
>
> Hi Jarkko,
>
> I noticed your commits https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=3ec9c3a0531ac868422be3b12fc17310ed8c07dc
> are no more referenced in your tree. Is there an issue?
This must be some sort of mistake I've made. I'll re-apply the patches.
Sorry about this.
BR, Jarkko
^ 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