[PATCH v2 01/12] ima: Don't ignore errors from crypto_shash_update()

[PATCH v2 01/12] ima: Don't ignore errors from crypto_shash_update()

[Roberto Sassu]

Errors returned by crypto_shash_update() are not checked in
ima_calc_boot_aggregate_tfm() and thus can be overwritten at the next
iteration of the loop. This patch adds a check after calling
crypto_shash_update() and returns immediately if the result is not zero.

Cc: stable@vger.kernel.org
Fixes: 3323eec921efd ("integrity: IMA as an integrity service provider")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_crypto.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 011c3c76af86..21989fa0c107 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -829,6 +829,8 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
 		/* now accumulate with current aggregate */
 		rc = crypto_shash_update(shash, d.digest,
 					 crypto_shash_digestsize(tfm));
+		if (rc != 0)
+			return rc;
 	}
 	/*
 	 * Extend cumulative digest over TPM registers 8-9, which contain
-- 
2.27.GIT

Re: [PATCH v2 01/12] ima: Don't ignore errors from crypto_shash_update()

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 3323eec921ef ("integrity: IMA as an integrity service provider").

The bot has tested the following trees: v5.8.7, v5.4.63, v4.19.143, v4.14.196, v4.9.235, v4.4.235.

v5.8.7: Build OK!
v5.4.63: Build OK!
v4.19.143: Failed to apply! Possible dependencies:
    100b16a6f290 ("tpm: sort objects in the Makefile")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    70a3199a7101 ("tpm: factor out tpm_get_timeouts()")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    95adc6b410b7 ("tpm: use u32 instead of int for PCR index")
    b03c43702e7b ("tpm: add tpm_auto_startup() into tpm-interface.c")
    b2d6e6de005e ("tpm: factor out tpm 1.x duration calculation to tpm1-cmd.c")
    c82a330ceced ("tpm: factor out tpm 1.x pm suspend flow into tpm1-cmd.c")
    d4a317563207 ("tpm: move tpm 1.x selftest code from tpm-interface.c tpm1-cmd.c")
    d856c00f7d16 ("tpm: add tpm_calc_ordinal_duration() wrapper")

v4.14.196: Failed to apply! Possible dependencies:
    5ef924d9e2e8 ("tpm: use tpm_msleep() value as max delay")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    95adc6b410b7 ("tpm: use u32 instead of int for PCR index")
    aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()")
    b03c43702e7b ("tpm: add tpm_auto_startup() into tpm-interface.c")
    c82a330ceced ("tpm: factor out tpm 1.x pm suspend flow into tpm1-cmd.c")
    d4a317563207 ("tpm: move tpm 1.x selftest code from tpm-interface.c tpm1-cmd.c")
    fc1d52b745ba ("tpm: rename tpm_chip_find_get() to tpm_find_get_ops()")

v4.9.235: Failed to apply! Possible dependencies:
    06e93279ca77 ("tpm: move endianness conversion of TPM_TAG_RQU_COMMAND to tpm_input_header")
    175d5b2a570c ("tpm: move TPM 1.2 code of tpm_pcr_extend() to tpm1_pcr_extend()")
    37f4915fef05 ("tpm: use idr_find(), not idr_find_slowpath()")
    51b0be640cf6 ("tpm: Fix expected number of response bytes of TPM1.2 PCR Extend")
    62bfdacbac4c ("tpm: Do not print an error message when doing TPM auto startup")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    84fda15286d1 ("tpm: sanitize constant expressions")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    a69faebf4d3e ("tpm: move endianness conversion of ordinals to tpm_input_header")
    aaa6f7f6c8bf ("tpm: Clean up reading of timeout and duration capabilities")
    aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()")
    c659af78eb7b ("tpm: Check size of response before accessing data")
    ca6d45802201 ("tpm: place kdoc just above tpm_pcr_extend")
    f865c196856d ("tpm: add kdoc for tpm_transmit and tpm_transmit_cmd")

v4.4.235: Failed to apply! Possible dependencies:
    0014777f989b ("tpm: constify TPM 1.x header structures")
    062807f20e3f ("tpm: Remove all uses of drvdata from the TPM Core")
    06e93279ca77 ("tpm: move endianness conversion of TPM_TAG_RQU_COMMAND to tpm_input_header")
    175d5b2a570c ("tpm: move TPM 1.2 code of tpm_pcr_extend() to tpm1_pcr_extend()")
    25112048cd59 ("tpm: rework tpm_get_timeouts()")
    3635e2ec7cbb ("tpm: Get rid of devname")
    37f4915fef05 ("tpm: use idr_find(), not idr_find_slowpath()")
    570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
    6e599f6f261f ("tpm: drop 'read_queue' from struct tpm_vendor_specific")
    6f1a1d103b48 ("ima: Switch to ima_hash_algo for boot aggregate")
    7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code")
    84fda15286d1 ("tpm: sanitize constant expressions")
    879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
    a69faebf4d3e ("tpm: move endianness conversion of ordinals to tpm_input_header")
    aad887f66411 ("tpm: use struct tpm_chip for tpm_chip_find_get()")
    af782f339a5d ("tpm: Move tpm_vendor_specific data related with PTP specification to tpm_chip")
    c659af78eb7b ("tpm: Check size of response before accessing data")
    ddab0e34288a ("tpm/st33zp24: Remove unneeded tpm_reg in get_burstcount")
    e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
    f865c196856d ("tpm: add kdoc for tpm_transmit and tpm_transmit_cmd")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

[PATCH v2 02/12] ima: Remove semicolon at the end of ima_get_binary_runtime_size()

[Roberto Sassu]

This patch removes the unnecessary semicolon at the end of
ima_get_binary_runtime_size().

Cc: stable@vger.kernel.org
Fixes: d158847ae89a2 ("ima: maintain memory size needed for serializing the measurement list")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_queue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index fb4ec270f620..c096ef8945c7 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -133,7 +133,7 @@ unsigned long ima_get_binary_runtime_size(void)
 		return ULONG_MAX;
 	else
 		return binary_runtime_size + sizeof(struct ima_kexec_hdr);
-};
+}
 
 static int ima_pcr_extend(struct tpm_digest *digests_arg, int pcr)
 {
-- 
2.27.GIT

[PATCH v2 03/12] evm: Check size of security.evm before using it

[Roberto Sassu]

This patch checks the size for the EVM_IMA_XATTR_DIGSIG and
EVM_XATTR_PORTABLE_DIGSIG types to ensure that the algorithm is read from
the buffer returned by vfs_getxattr_alloc().

Cc: stable@vger.kernel.org # 4.19.x
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/evm/evm_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 0d36259b690d..e4b47759ba1c 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -181,6 +181,12 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
 	case EVM_XATTR_PORTABLE_DIGSIG:
+		/* accept xattr with non-empty signature field */
+		if (xattr_len <= sizeof(struct signature_v2_hdr)) {
+			evm_status = INTEGRITY_FAIL;
+			goto out;
+		}
+
 		hdr = (struct signature_v2_hdr *)xattr_data;
 		digest.hdr.algo = hdr->hash_algo;
 		rc = evm_calc_hash(dentry, xattr_name, xattr_value,
-- 
2.27.GIT

[PATCH v2 04/12] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Roberto Sassu]

evm_inode_init_security() requires the HMAC key to calculate the HMAC on
initial xattrs provided by LSMs. Unfortunately, with the evm_key_loaded()
check, the function continues even if the HMAC key is not loaded
(evm_key_loaded() returns true also if EVM has been initialized only with a
public key). If the HMAC key is not loaded, evm_inode_init_security()
returns an error later when it calls evm_init_hmac().

Thus, this patch replaces the evm_key_loaded() check with a check of the
EVM_INIT_HMAC flag in evm_initialized, so that evm_inode_init_security()
returns 0 instead of an error.

Cc: stable@vger.kernel.org # 4.5.x
Fixes: 26ddabfe96b ("evm: enable EVM when X509 certificate is loaded")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/evm/evm_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index e4b47759ba1c..4e9f5e8b21d5 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -527,7 +527,8 @@ int evm_inode_init_security(struct inode *inode,
 	struct evm_xattr *xattr_data;
 	int rc;
 
-	if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
+	if (!(evm_initialized & EVM_INIT_HMAC) ||
+	    !evm_protected_xattr(lsm_xattr->name))
 		return 0;
 
 	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
-- 
2.27.GIT

Re: [PATCH v2 04/12] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 26ddabfe96bb ("evm: enable EVM when X509 certificate is loaded").

The bot has tested the following trees: v5.8.7, v5.4.63, v4.19.143, v4.14.196, v4.9.235.

v5.8.7: Build OK!
v5.4.63: Build OK!
v4.19.143: Build OK!
v4.14.196: Failed to apply! Possible dependencies:
    21af76631476 ("EVM: turn evm_config_xattrnames into a list")
    5feeb61183dd ("evm: Allow non-SHA1 digital signatures")
    650b29dbdf2c ("integrity: Introduce struct evm_xattr")
    ae1ba1676b88 ("EVM: Allow userland to permit modification of EVM-protected metadata")
    b33e3cc5c90b ("Merge branch 'next-integrity' of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security")
    f00d79750712 ("EVM: Allow userspace to signal an RSA key has been loaded")

v4.9.235: Failed to apply! Possible dependencies:
    21af76631476 ("EVM: turn evm_config_xattrnames into a list")
    5feeb61183dd ("evm: Allow non-SHA1 digital signatures")
    650b29dbdf2c ("integrity: Introduce struct evm_xattr")
    ae1ba1676b88 ("EVM: Allow userland to permit modification of EVM-protected metadata")
    b33e3cc5c90b ("Merge branch 'next-integrity' of git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security")
    b4bfec7f4a86 ("security/integrity: Harden against malformed xattrs")
    f00d79750712 ("EVM: Allow userspace to signal an RSA key has been loaded")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks
Sasha

Re: [PATCH v2 04/12] evm: Execute evm_inode_init_security() only when the HMAC key is loaded

[Mimi Zohar]

Hi Roberto,

On Fri, 2020-09-04 at 11:23 +0200, Roberto Sassu wrote:
> evm_inode_init_security() requires the HMAC key to calculate the HMAC on
> initial xattrs provided by LSMs. Unfortunately, with the evm_key_loaded()
> check, the function continues even if the HMAC key is not loaded
> (evm_key_loaded() returns true also if EVM has been initialized only with a
> public key). If the HMAC key is not loaded, evm_inode_init_security()
> returns an error later when it calls evm_init_hmac().

This is all true, but the context for why it wasn't an issue previously
is missing.

The original usecase for allowing signature verificaton prior to
loading the HMAC key was a fully signed, possibly immutable, initrd. 
No new files were created or, at least, were in policy until the HMAC
key was loaded.   More recently support for requiring an EVM HMAC key
was removed.  Files having a portable and immutable signature were
given additional privileges.

Please update the patch description with the context of what has
changed.

Mimi

[PATCH v2 06/12] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded

[Roberto Sassu]

EVM_ALLOW_METADATA_WRITES is an EVM initialization flag that can be set to
temporarily disable metadata verification until all xattrs/attrs necessary
to verify an EVM portable signature are copied to the file. This flag is
cleared when EVM is initialized with an HMAC key, to avoid that the HMAC is
calculated on unverified xattrs/attrs.

Currently EVM unnecessarily denies setting this flag if EVM is initialized
with public key, which is not a concern as it cannot be used to trust
xattrs/attrs updates. This patch removes this limitation.

Cc: stable@vger.kernel.org # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 Documentation/ABI/testing/evm      | 6 ++++--
 security/integrity/evm/evm_secfs.c | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/evm b/Documentation/ABI/testing/evm
index 201d10319fa1..cbb50ab09c78 100644
--- a/Documentation/ABI/testing/evm
+++ b/Documentation/ABI/testing/evm
@@ -42,8 +42,10 @@ Description:
 		modification of EVM-protected metadata and
 		disable all further modification of policy
 
-		Note that once a key has been loaded, it will no longer be
-		possible to enable metadata modification.
+		Note that once HMAC validation and creation is enabled,
+		it will no longer be possible to enable metadata modification
+		and if metadata modification is already enabled, it will be
+		disabled.
 
 		Until key loading has been signaled EVM can not create
 		or validate the 'security.evm' xattr, but returns
diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index cfc3075769bb..92fe26ace797 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
 	 * keys are loaded.
 	 */
 	if ((i & EVM_ALLOW_METADATA_WRITES) &&
-	    ((evm_initialized & EVM_KEY_MASK) != 0) &&
+	    ((evm_initialized & EVM_INIT_HMAC) != 0) &&
 	    !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
 		return -EPERM;
 
-- 
2.27.GIT

[PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Roberto Sassu]

When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
metadata. Its main purpose is to allow users to freely set metadata when
they are protected by a portable signature, until the HMAC key is loaded.

However, IMA is not notified about metadata changes and, after the first
successful appraisal, always allows access to the files without checking
metadata again.

This patch introduces the new atomic flag EVM_RESET_STATUS in
integrity_iint_cache that is set in the EVM post hooks and cleared in
evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
it is set, it clears the appraisal flags.

Although the flag could be cleared also by evm_inode_setxattr() and
evm_inode_setattr() before IMA sees it, this does not happen if
EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
evm_verifyxattr(), this ensures that IMA always sees the flag set before it
is cleared.

This patch also adds a call to evm_reset_status() in
evm_inode_post_setattr() so that EVM won't return the cached status the
next time appraisal is performed.

Cc: stable@vger.kernel.org # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 17 +++++++++++++++--
 security/integrity/ima/ima_main.c |  8 ++++++--
 security/integrity/integrity.h    |  1 +
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 4e9f5e8b21d5..05be1ad3e6f3 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -221,8 +221,15 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		evm_status = (rc == -ENODATA) ?
 				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
 out:
-	if (iint)
+	if (iint) {
+		/*
+		 * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
+		 * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
+		 * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
+		 */
+		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
 		iint->evm_status = evm_status;
+	}
 	kfree(xattr_data);
 	return evm_status;
 }
@@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
 	struct integrity_iint_cache *iint;
 
 	iint = integrity_iint_find(inode);
-	if (iint)
+	if (iint) {
+		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
+			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
+
 		iint->evm_status = INTEGRITY_UNKNOWN;
+	}
 }
 
 /**
@@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 	if (!evm_key_loaded())
 		return;
 
+	evm_reset_status(dentry->d_inode);
+
 	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		evm_update_evmxattr(dentry, NULL, NULL, 0);
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..bb9976dc2b74 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -246,8 +246,12 @@ static int process_measurement(struct file *file, const struct cred *cred,
 
 	mutex_lock(&iint->mutex);
 
-	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
-		/* reset appraisal flags if ima_inode_post_setattr was called */
+	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags) ||
+	    test_bit(EVM_RESET_STATUS, &iint->atomic_flags))
+		/*
+		 * Reset appraisal flags if ima_inode_post_setattr was called or
+		 * EVM reset its status and metadata modification was enabled.
+		 */
 		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
 				 IMA_ACTION_FLAGS);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 413c803c5208..2adec51c0f6e 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -70,6 +70,7 @@
 #define IMA_CHANGE_ATTR		2
 #define IMA_DIGSIG		3
 #define IMA_MUST_MEASURE	4
+#define EVM_RESET_STATUS	5
 
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,
-- 
2.27.GIT

Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Mimi Zohar]

[Cc'ing John Johansen]

Hi Roberto,

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
> 
> However, IMA is not notified about metadata changes and, after the first
> successful appraisal, always allows access to the files without checking
> metadata again.
> 
> This patch introduces the new atomic flag EVM_RESET_STATUS in
> integrity_iint_cache that is set in the EVM post hooks and cleared in
> evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
> it is set, it clears the appraisal flags.
> 
> Although the flag could be cleared also by evm_inode_setxattr() and
> evm_inode_setattr() before IMA sees it, this does not happen if
> EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> is cleared.
> 
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
> 
> Cc: stable@vger.kernel.org # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 17 +++++++++++++++--
>  security/integrity/ima/ima_main.c |  8 ++++++--
>  security/integrity/integrity.h    |  1 +
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 4e9f5e8b21d5..05be1ad3e6f3 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -221,8 +221,15 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  		evm_status = (rc == -ENODATA) ?
>  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
>  out:
> -	if (iint)
> +	if (iint) {
> +		/*
> +		 * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
> +		 * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
> +		 * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
> +		 */
> +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
>  		iint->evm_status = evm_status;

True IMA is currently the only caller of evm_verifyxattr() in the
upstreamed kernel, but it is an exported function, which may be called
from elsewhere.  The previous version crossed the boundary between EVM
& IMA with EVM modifying the IMA flag directly.  This version assumes
that IMA will be the only caller.  Otherwise, I like this version.

Mimi

> +	}
>  	kfree(xattr_data);
>  	return evm_status;
>  }
> @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
>  	struct integrity_iint_cache *iint;
>  
>  	iint = integrity_iint_find(inode);
> -	if (iint)
> +	if (iint) {
> +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> +			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> +
>  		iint->evm_status = INTEGRITY_UNKNOWN;
> +	}
>  }
>  
>  /**
> @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>  	if (!evm_key_loaded())
>  		return;
>  
> +	evm_reset_status(dentry->d_inode);
> +
>  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>  		evm_update_evmxattr(dentry, NULL, NULL, 0);
>  }

RE: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Roberto Sassu]

> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, September 17, 2020 2:01 PM
> [Cc'ing John Johansen]
> 
> Hi Roberto,
> 
> On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> on
> > metadata. Its main purpose is to allow users to freely set metadata when
> > they are protected by a portable signature, until the HMAC key is loaded.
> >
> > However, IMA is not notified about metadata changes and, after the first
> > successful appraisal, always allows access to the files without checking
> > metadata again.
> >
> > This patch introduces the new atomic flag EVM_RESET_STATUS in
> > integrity_iint_cache that is set in the EVM post hooks and cleared in
> > evm_verify_hmac(). IMA checks the new flag in process_measurement()
> and if
> > it is set, it clears the appraisal flags.
> >
> > Although the flag could be cleared also by evm_inode_setxattr() and
> > evm_inode_setattr() before IMA sees it, this does not happen if
> > EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> > evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> > is cleared.
> >
> > This patch also adds a call to evm_reset_status() in
> > evm_inode_post_setattr() so that EVM won't return the cached status
> the
> > next time appraisal is performed.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_main.c | 17 +++++++++++++++--
> >  security/integrity/ima/ima_main.c |  8 ++++++--
> >  security/integrity/integrity.h    |  1 +
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -221,8 +221,15 @@ static enum integrity_status
> evm_verify_hmac(struct dentry *dentry,
> >  		evm_status = (rc == -ENODATA) ?
> >  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> >  out:
> > -	if (iint)
> > +	if (iint) {
> > +		/*
> > +		 * EVM_RESET_STATUS can be cleared only by
> evm_verifyxattr()
> > +		 * when EVM_ALLOW_METADATA_WRITES is set. This
> guarantees that
> > +		 * IMA sees the EVM_RESET_STATUS flag set before it is
> cleared.
> > +		 */
> > +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> >  		iint->evm_status = evm_status;
> 
> True IMA is currently the only caller of evm_verifyxattr() in the
> upstreamed kernel, but it is an exported function, which may be called
> from elsewhere.  The previous version crossed the boundary between EVM
> & IMA with EVM modifying the IMA flag directly.  This version assumes
> that IMA will be the only caller.  Otherwise, I like this version.

Ok, I think it is better, as you suggested, to export a new EVM function
that tells if evm_reset_status() will be executed in the EVM post hooks, and
to call this function from IMA. IMA would then call ima_reset_appraise_flags()
also depending on the result of the new EVM function.

ima_reset_appraise_flags() should be called in a post hook in IMA.
Should I introduce it?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> Mimi
> 
> > +	}
> >  	kfree(xattr_data);
> >  	return evm_status;
> >  }
> > @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
> >  	struct integrity_iint_cache *iint;
> >
> >  	iint = integrity_iint_find(inode);
> > -	if (iint)
> > +	if (iint) {
> > +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> > +			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > +
> >  		iint->evm_status = INTEGRITY_UNKNOWN;
> > +	}
> >  }
> >
> >  /**
> > @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
> >  	if (!evm_key_loaded())
> >  		return;
> >
> > +	evm_reset_status(dentry->d_inode);
> > +
> >  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> >  		evm_update_evmxattr(dentry, NULL, NULL, 0);
> >  }

Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

[Mimi Zohar]

On Thu, 2020-09-17 at 17:36 +0000, Roberto Sassu wrote:
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -221,8 +221,15 @@ static enum integrity_status
> > evm_verify_hmac(struct dentry *dentry,
> > >  		evm_status = (rc == -ENODATA) ?
> > >  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> > >  out:
> > > -	if (iint)
> > > +	if (iint) {
> > > +		/*
> > > +		 * EVM_RESET_STATUS can be cleared only by
> > evm_verifyxattr()
> > > +		 * when EVM_ALLOW_METADATA_WRITES is set. This
> > guarantees that
> > > +		 * IMA sees the EVM_RESET_STATUS flag set before it is
> > cleared.
> > > +		 */
> > > +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > >  		iint->evm_status = evm_status;
> > 
> > True IMA is currently the only caller of evm_verifyxattr() in the
> > upstreamed kernel, but it is an exported function, which may be called
> > from elsewhere.  The previous version crossed the boundary between EVM
> > & IMA with EVM modifying the IMA flag directly.  This version assumes
> > that IMA will be the only caller.  Otherwise, I like this version.
> 
> Ok, I think it is better, as you suggested, to export a new EVM function
> that tells if evm_reset_status() will be executed in the EVM post hooks, and
> to call this function from IMA. IMA would then call ima_reset_appraise_flags()
> also depending on the result of the new EVM function.
> 
> ima_reset_appraise_flags() should be called in a post hook in IMA.
> Should I introduce it?

Yes, so any callers of evm_verifyxattr() will need to implement the
post hook as well.  As much as possible, please limit code duplication.

The last time I looked, there didn't seem to be a locking concern, but
please make sure.

thanks,

Mimi