[PATCH 0/5] evm: Support signatures on stacked filesystem

[PATCH 0/5] evm: Support signatures on stacked filesystem

[Stefan Berger]

EVM has recently been completely disabled on unsupported (e.g.,
overlayfs). This series now enables copy-up of "portable and immutable"
signatures on those filesystems and enables the enforcement of
"portable and immutable" as well as the "original" signatures on
previously unsupported filesystem when EVM is enabled with EVM_INIT_X509.
HMAC verification and generation remains disabled on those filesystems.

Regards,
   Stefan

Stefan Berger (5):
  security: allow finer granularity in permitting copy-up of security
    xattrs
  evm: Implement per signature type decision in
    security_inode_copy_up_xattr
  ima: Reset EVM status upon detecting changes to overlay backing file
  evm: Use the real inode's metadata to calculate metadata hash
  evm: Enforce signatures on unsupported filesystem for EVM_INIT_X509

 fs/overlayfs/copy_up.c              |  2 +-
 include/linux/evm.h                 | 10 +++++-
 include/linux/lsm_hook_defs.h       |  3 +-
 include/linux/security.h            |  4 +--
 security/integrity/evm/evm_crypto.c |  2 +-
 security/integrity/evm/evm_main.c   | 48 +++++++++++++++++++++++------
 security/integrity/ima/ima_main.c   |  2 ++
 security/security.c                 |  7 +++--
 security/selinux/hooks.c            |  2 +-
 security/smack/smack_lsm.c          |  2 +-
 10 files changed, 62 insertions(+), 20 deletions(-)

-- 
2.43.0

[PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Stefan Berger]

Copying up xattrs is solely based on the security xattr name. For finer
granularity add a dentry parameter to the security_inode_copy_up_xattr
hook definition, allowing decisions to be based on the xattr content as
well.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 fs/overlayfs/copy_up.c            | 2 +-
 include/linux/evm.h               | 2 +-
 include/linux/lsm_hook_defs.h     | 3 ++-
 include/linux/security.h          | 4 ++--
 security/integrity/evm/evm_main.c | 2 +-
 security/security.c               | 7 ++++---
 security/selinux/hooks.c          | 2 +-
 security/smack/smack_lsm.c        | 2 +-
 8 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b8e25ca51016..bd9ddcefb7a7 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
 		if (ovl_is_private_xattr(sb, name))
 			continue;
 
-		error = security_inode_copy_up_xattr(name);
+		error = security_inode_copy_up_xattr(old, name);
 		if (error < 0 && error != -EOPNOTSUPP)
 			break;
 		if (error == 1) {
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 36ec884320d9..d8c0343436b8 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -31,7 +31,7 @@ extern void evm_inode_post_setxattr(struct dentry *dentry,
 				    const char *xattr_name,
 				    const void *xattr_value,
 				    size_t xattr_value_len);
-extern int evm_inode_copy_up_xattr(const char *name);
+extern int evm_inode_copy_up_xattr(struct dentry *dentry, const char *name);
 extern int evm_inode_removexattr(struct mnt_idmap *idmap,
 				 struct dentry *dentry, const char *xattr_name);
 extern void evm_inode_post_removexattr(struct dentry *dentry,
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 185924c56378..7dd61f51d84a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -163,7 +163,8 @@ LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
 	 size_t buffer_size)
 LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
 LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
-LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, const char *name)
+LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, struct dentry *src,
+	 const char *name)
 LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
 	 struct kernfs_node *kn)
 LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..9fc9ca6284d6 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -387,7 +387,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
 int security_inode_copy_up(struct dentry *src, struct cred **new);
-int security_inode_copy_up_xattr(const char *name);
+int security_inode_copy_up_xattr(struct dentry *src, const char *name);
 int security_kernfs_init_security(struct kernfs_node *kn_dir,
 				  struct kernfs_node *kn);
 int security_file_permission(struct file *file, int mask);
@@ -980,7 +980,7 @@ static inline int security_kernfs_init_security(struct kernfs_node *kn_dir,
 	return 0;
 }
 
-static inline int security_inode_copy_up_xattr(const char *name)
+static inline int security_inode_copy_up_xattr(struct dentry *src, const char *name)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index cc7956d7878b..2555aa4501ae 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -896,7 +896,7 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 		evm_update_evmxattr(dentry, NULL, NULL, 0);
 }
 
-int evm_inode_copy_up_xattr(const char *name)
+int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
 {
 	if (strcmp(name, XATTR_NAME_EVM) == 0)
 		return 1; /* Discard */
diff --git a/security/security.c b/security/security.c
index 0144a98d3712..ee63863c1dc0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2596,6 +2596,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
 
 /**
  * security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
+ * @src: union dentry of copy-up file
  * @name: xattr name
  *
  * Filter the xattrs being copied up when a unioned file is copied up from a
@@ -2606,7 +2607,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
  *         if the security module does not know about attribute, or a negative
  *         error code to abort the copy up.
  */
-int security_inode_copy_up_xattr(const char *name)
+int security_inode_copy_up_xattr(struct dentry *src, const char *name)
 {
 	struct security_hook_list *hp;
 	int rc;
@@ -2618,12 +2619,12 @@ int security_inode_copy_up_xattr(const char *name)
 	 */
 	hlist_for_each_entry(hp,
 			     &security_hook_heads.inode_copy_up_xattr, list) {
-		rc = hp->hook.inode_copy_up_xattr(name);
+		rc = hp->hook.inode_copy_up_xattr(src, name);
 		if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
 			return rc;
 	}
 
-	return evm_inode_copy_up_xattr(name);
+	return evm_inode_copy_up_xattr(src, name);
 }
 EXPORT_SYMBOL(security_inode_copy_up_xattr);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..ebb8876837c6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3530,7 +3530,7 @@ static int selinux_inode_copy_up(struct dentry *src, struct cred **new)
 	return 0;
 }
 
-static int selinux_inode_copy_up_xattr(const char *name)
+static int selinux_inode_copy_up_xattr(struct dentry *dentry, const char *name)
 {
 	/* The copy_up hook above sets the initial context on an inode, but we
 	 * don't then want to overwrite it by blindly copying all the lower
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..bffca165f07f 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4873,7 +4873,7 @@ static int smack_inode_copy_up(struct dentry *dentry, struct cred **new)
 	return 0;
 }
 
-static int smack_inode_copy_up_xattr(const char *name)
+static int smack_inode_copy_up_xattr(struct dentry *src, const char *name)
 {
 	/*
 	 * Return 1 if this is the smack access Smack attribute.
-- 
2.43.0

[PATCH 2/5] evm: Implement per signature type decision in security_inode_copy_up_xattr

[Stefan Berger]

To support portable and immutable signatures on otherwise unsupported
filesystems, determine the EVM signature type by the content of a file's
xattr. If the file has the appropriate signature then allow it to be
copied up. All other signature types are discarded as before.

Portable and immutable EVM signatures can be copied up by stacked file-
system since the metadata their signature covers does not include file-
system-specific data such as a file's inode number, generation, and UUID.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 security/integrity/evm/evm_main.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2555aa4501ae..22a5e26860ea 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -898,9 +898,30 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 
 int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
 {
-	if (strcmp(name, XATTR_NAME_EVM) == 0)
-		return 1; /* Discard */
-	return -EOPNOTSUPP;
+	struct evm_ima_xattr_data *xattr_data = NULL;
+	int rc;
+
+	if (strcmp(name, XATTR_NAME_EVM) != 0)
+		return -EOPNOTSUPP;
+
+	/* first need to know the sig type */
+	rc = vfs_getxattr_alloc(&nop_mnt_idmap, src, XATTR_NAME_EVM,
+				(char **)&xattr_data, 0, GFP_NOFS);
+	if (rc <= 0)
+		return -EPERM;
+
+	switch (xattr_data->type) {
+	case EVM_XATTR_PORTABLE_DIGSIG:
+		rc = 0; /* allow copy-up */
+		break;
+	case EVM_XATTR_HMAC:
+	case EVM_IMA_XATTR_DIGSIG:
+	default:
+		rc = 1; /* discard */
+	}
+
+	kfree(xattr_data);
+	return rc;
 }
 
 /*
-- 
2.43.0

[PATCH 3/5] ima: Reset EVM status upon detecting changes to overlay backing file

[Stefan Berger]

To avoid caching effects to take effect reset the EVM status upon
detecting changes to the overlay backing files. This prevents a not-yet-
copied-up file on the overlay from executing if for example the
security.evm xattr on the file on the 'lower' layer has been removed.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 include/linux/evm.h               | 8 ++++++++
 security/integrity/evm/evm_main.c | 7 +++++++
 security/integrity/ima/ima_main.c | 2 ++
 3 files changed, 17 insertions(+)

diff --git a/include/linux/evm.h b/include/linux/evm.h
index d8c0343436b8..e7d6742eee9d 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -66,6 +66,8 @@ extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
 extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
 				     int buffer_size, char type,
 				     bool canonical_fmt);
+extern void evm_reset_cache_status(struct dentry *dentry,
+				   struct integrity_iint_cache *iint);
 #ifdef CONFIG_FS_POSIX_ACL
 extern int posix_xattr_acl(const char *xattrname);
 #else
@@ -189,5 +191,11 @@ static inline int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
 	return -EOPNOTSUPP;
 }
 
+static inline void evm_reset_cache_status(struct dentry *dentry,
+					  struct integrity_iint_cache *iint)
+{
+	return;
+}
+
 #endif /* CONFIG_EVM */
 #endif /* LINUX_EVM_H */
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 22a5e26860ea..e96d127b48a2 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -721,6 +721,13 @@ static void evm_reset_status(struct inode *inode)
 		iint->evm_status = INTEGRITY_UNKNOWN;
 }
 
+void evm_reset_cache_status(struct dentry *dentry,
+			    struct integrity_iint_cache *iint)
+{
+	if (d_real_inode(dentry) != d_backing_inode(dentry))
+		iint->evm_status = INTEGRITY_UNKNOWN;
+}
+
 /**
  * evm_revalidate_status - report whether EVM status re-validation is necessary
  * @xattr_name: pointer to the affected extended attribute name
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cc1217ac2c6f..84bdc6e58329 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -26,6 +26,7 @@
 #include <linux/ima.h>
 #include <linux/fs.h>
 #include <linux/iversion.h>
+#include <linux/evm.h>
 
 #include "ima.h"
 
@@ -295,6 +296,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		    !inode_eq_iversion(backing_inode, iint->version)) {
 			iint->flags &= ~IMA_DONE_MASK;
 			iint->measured_pcrs = 0;
+			evm_reset_cache_status(file_dentry(file), iint);
 		}
 	}
 
-- 
2.43.0

[PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

Changes to the file attribute (mode bits, uid, gid) on the lower layer
are not take into account when d_backing_inode() is used when a file is
accessed on the overlay layer and this file has not yet been copied up.
This is because d_backing_inode() does not return the real inode of the
lower layer but instead returns the backing inode which holds old file
attributes. When the old file attributes are used for calculating the
metadata hash then the expected hash is calculated and the file then
mistakenly passes signature verification. Therefore, use d_real_inode()
which returns the inode of the lower layer for as long as the file has
not been copied up and returns the upper layer's inode otherwise.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 security/integrity/evm/evm_crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index b1ffd4cc0b44..2e48fe54e899 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
 				 size_t req_xattr_value_len,
 				 uint8_t type, struct evm_digest *data)
 {
-	struct inode *inode = d_backing_inode(dentry);
+	struct inode *inode = d_real_inode(dentry);
 	struct xattr_list *xattr;
 	struct shash_desc *desc;
 	size_t xattr_size = 0;
-- 
2.43.0

[PATCH 5/5] evm: Enforce signatures on unsupported filesystem for EVM_INIT_X509

[Stefan Berger]

Unsupported filesystems currently do not enforce any signatures. Add
support for signature enforcement of the "original" and "portable &
immutable" signatures when EVM_INIT_X509 is enabled.

The "original" signature type contains filesystem specific metadata.
Thus it cannot be copied up and verified. However with EVM_INIT_X509
and EVM_ALLOW_METADATA_WRITES enabled, the "original" file signature
may be written.

When EVM_ALLOW_METADATA_WRITES is not set or once it is removed from
/sys/kernel/security/evm by setting EVM_INIT_HMAC for example, it is not
possible to write or remove xattrs on the overlay filesystem.

This change still prevents EVM from writing HMAC signatures on
unsupported filesystem when EVM_INIT_HMAC is enabled.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 security/integrity/evm/evm_main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index e96d127b48a2..f49609dfcbc7 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -192,7 +192,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		     iint->evm_status == INTEGRITY_PASS_IMMUTABLE))
 		return iint->evm_status;
 
-	if (is_unsupported_fs(dentry))
+	/*
+	 * On unsupported filesystems with EVM_INIT_X509 not enabled, skip
+	 * signature verification.
+	 */
+	if (!(evm_initialized & EVM_INIT_X509) && is_unsupported_fs(dentry))
 		return INTEGRITY_UNKNOWN;
 
 	/* if status is not PASS, try to check again - against -ENOMEM */
@@ -262,7 +266,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 				evm_status = INTEGRITY_PASS_IMMUTABLE;
 			} else if (!IS_RDONLY(inode) &&
 				   !(inode->i_sb->s_readonly_remount) &&
-				   !IS_IMMUTABLE(inode)) {
+				   !IS_IMMUTABLE(inode) &&
+				   !is_unsupported_fs(dentry)) {
 				evm_update_evmxattr(dentry, xattr_name,
 						    xattr_value,
 						    xattr_value_len);
@@ -422,9 +427,6 @@ enum integrity_status evm_verifyxattr(struct dentry *dentry,
 	if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
 		return INTEGRITY_UNKNOWN;
 
-	if (is_unsupported_fs(dentry))
-		return INTEGRITY_UNKNOWN;
-
 	if (!iint) {
 		iint = integrity_iint_find(d_backing_inode(dentry));
 		if (!iint)
-- 
2.43.0

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 1/30/24 16:46, Stefan Berger wrote:
> Changes to the file attribute (mode bits, uid, gid) on the lower layer
> are not take into account when d_backing_inode() is used when a file is
> accessed on the overlay layer and this file has not yet been copied up.
> This is because d_backing_inode() does not return the real inode of the
> lower layer but instead returns the backing inode which holds old file
> attributes. When the old file attributes are used for calculating the
> metadata hash then the expected hash is calculated and the file then
> mistakenly passes signature verification. Therefore, use d_real_inode()
> which returns the inode of the lower layer for as long as the file has
> not been copied up and returns the upper layer's inode otherwise.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   security/integrity/evm/evm_crypto.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index b1ffd4cc0b44..2e48fe54e899 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>   				 size_t req_xattr_value_len,
>   				 uint8_t type, struct evm_digest *data)
>   {
> -	struct inode *inode = d_backing_inode(dentry);
> +	struct inode *inode = d_real_inode(dentry);
>   	struct xattr_list *xattr;
>   	struct shash_desc *desc;
>   	size_t xattr_size = 0;

We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but 
when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am 
not sure what the solution is.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 1/30/24 16:46, Stefan Berger wrote:
> > Changes to the file attribute (mode bits, uid, gid) on the lower layer
> > are not take into account when d_backing_inode() is used when a file is
> > accessed on the overlay layer and this file has not yet been copied up.
> > This is because d_backing_inode() does not return the real inode of the
> > lower layer but instead returns the backing inode which holds old file
> > attributes. When the old file attributes are used for calculating the
> > metadata hash then the expected hash is calculated and the file then
> > mistakenly passes signature verification. Therefore, use d_real_inode()
> > which returns the inode of the lower layer for as long as the file has
> > not been copied up and returns the upper layer's inode otherwise.
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >   security/integrity/evm/evm_crypto.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index b1ffd4cc0b44..2e48fe54e899 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> >                                size_t req_xattr_value_len,
> >                                uint8_t type, struct evm_digest *data)
> >   {
> > -     struct inode *inode = d_backing_inode(dentry);
> > +     struct inode *inode = d_real_inode(dentry);
> >       struct xattr_list *xattr;
> >       struct shash_desc *desc;
> >       size_t xattr_size = 0;
>
> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
> not sure what the solution is.

I think d_real_inode() does not work correctly for all its current users for
a metacopy file.

I think the solution is to change d_real_inode() to return the data inode
and add another helper to get the metadata inode if needed.
I will post some patches for it.

However, I must say that I do not know if evm_calc_hmac_or_hash()
needs the lower data inode, the upper metadata inode or both.

The last time you tried to fix ovl+IMA, I asked for documentation
of what data/metadata is protected with EVM and how are those
protections supposed to work across overlayfs copy up, when the
data and metadata are often split between 2 and myabe event 3
differnt inode.

From the current patch set, I still don't understand what is the expected
behavior before and after copy up of data/metadata-only.

Thanks,
Amir.

Re: [PATCH 0/5] evm: Support signatures on stacked filesystem

[Amir Goldstein]

On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> EVM has recently been completely disabled on unsupported (e.g.,
> overlayfs). This series now enables copy-up of "portable and immutable"
> signatures on those filesystems and enables the enforcement of
> "portable and immutable" as well as the "original" signatures on
> previously unsupported filesystem when EVM is enabled with EVM_INIT_X509.
> HMAC verification and generation remains disabled on those filesystems.
>

I am missing a high level description of what is in those "portable
and immutable"
signatures and how those signatures remain valid across copy up.

Thanks,
Amir.

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Amir Goldstein]

On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> Copying up xattrs is solely based on the security xattr name. For finer
> granularity add a dentry parameter to the security_inode_copy_up_xattr
> hook definition, allowing decisions to be based on the xattr content as
> well.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  fs/overlayfs/copy_up.c            | 2 +-
>  include/linux/evm.h               | 2 +-
>  include/linux/lsm_hook_defs.h     | 3 ++-
>  include/linux/security.h          | 4 ++--
>  security/integrity/evm/evm_main.c | 2 +-
>  security/security.c               | 7 ++++---
>  security/selinux/hooks.c          | 2 +-
>  security/smack/smack_lsm.c        | 2 +-
>  8 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index b8e25ca51016..bd9ddcefb7a7 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
>                 if (ovl_is_private_xattr(sb, name))
>                         continue;
>
> -               error = security_inode_copy_up_xattr(name);
> +               error = security_inode_copy_up_xattr(old, name);

What do you think about:

                     error = security_inode_copy_up_xattr(name, NULL, 0);

and then later...

                     error = security_inode_copy_up_xattr(name, value, size);

I am asking because overlayfs uses mnt_idmap(path->mnt) and you
have used nop_mnt_idmap inside evm hook.
this does not look right to me?

Thanks,
Amir.

Re: [PATCH 2/5] evm: Implement per signature type decision in security_inode_copy_up_xattr

[Amir Goldstein]

On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> To support portable and immutable signatures on otherwise unsupported
> filesystems, determine the EVM signature type by the content of a file's
> xattr. If the file has the appropriate signature then allow it to be
> copied up. All other signature types are discarded as before.
>
> Portable and immutable EVM signatures can be copied up by stacked file-
> system since the metadata their signature covers does not include file-
> system-specific data such as a file's inode number, generation, and UUID.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  security/integrity/evm/evm_main.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 2555aa4501ae..22a5e26860ea 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -898,9 +898,30 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>
>  int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
>  {
> -       if (strcmp(name, XATTR_NAME_EVM) == 0)
> -               return 1; /* Discard */
> -       return -EOPNOTSUPP;
> +       struct evm_ima_xattr_data *xattr_data = NULL;
> +       int rc;
> +
> +       if (strcmp(name, XATTR_NAME_EVM) != 0)
> +               return -EOPNOTSUPP;
> +
> +       /* first need to know the sig type */
> +       rc = vfs_getxattr_alloc(&nop_mnt_idmap, src, XATTR_NAME_EVM,
> +                               (char **)&xattr_data, 0, GFP_NOFS);


See my suggestion for post-getxattr hook:
security_inode_copy_up_xattr(name, value, size)
to avoid using nop_mnt_idmap here.

Unless it is fine to use nop_mnt_idmap in this context? not sure.

Thanks,
Amir.

Re: [PATCH 3/5] ima: Reset EVM status upon detecting changes to overlay backing file

[Amir Goldstein]

On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> To avoid caching effects to take effect reset the EVM status upon
> detecting changes to the overlay backing files. This prevents a not-yet-
> copied-up file on the overlay from executing if for example the
> security.evm xattr on the file on the 'lower' layer has been removed.
>

And what is expected to happen when file is executed after copy up?
Doesn't this change also protect the same threat after copy up?

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  include/linux/evm.h               | 8 ++++++++
>  security/integrity/evm/evm_main.c | 7 +++++++
>  security/integrity/ima/ima_main.c | 2 ++
>  3 files changed, 17 insertions(+)
>
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index d8c0343436b8..e7d6742eee9d 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -66,6 +66,8 @@ extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
>  extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>                                      int buffer_size, char type,
>                                      bool canonical_fmt);
> +extern void evm_reset_cache_status(struct dentry *dentry,
> +                                  struct integrity_iint_cache *iint);
>  #ifdef CONFIG_FS_POSIX_ACL
>  extern int posix_xattr_acl(const char *xattrname);
>  #else
> @@ -189,5 +191,11 @@ static inline int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>         return -EOPNOTSUPP;
>  }
>
> +static inline void evm_reset_cache_status(struct dentry *dentry,
> +                                         struct integrity_iint_cache *iint)
> +{
> +       return;
> +}
> +
>  #endif /* CONFIG_EVM */
>  #endif /* LINUX_EVM_H */
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 22a5e26860ea..e96d127b48a2 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -721,6 +721,13 @@ static void evm_reset_status(struct inode *inode)
>                 iint->evm_status = INTEGRITY_UNKNOWN;
>  }
>
> +void evm_reset_cache_status(struct dentry *dentry,
> +                           struct integrity_iint_cache *iint)
> +{
> +       if (d_real_inode(dentry) != d_backing_inode(dentry))
> +               iint->evm_status = INTEGRITY_UNKNOWN;
> +}
> +
>  /**
>   * evm_revalidate_status - report whether EVM status re-validation is necessary
>   * @xattr_name: pointer to the affected extended attribute name
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index cc1217ac2c6f..84bdc6e58329 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -26,6 +26,7 @@
>  #include <linux/ima.h>
>  #include <linux/fs.h>
>  #include <linux/iversion.h>
> +#include <linux/evm.h>
>
>  #include "ima.h"
>
> @@ -295,6 +296,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>                     !inode_eq_iversion(backing_inode, iint->version)) {
>                         iint->flags &= ~IMA_DONE_MASK;
>                         iint->measured_pcrs = 0;
> +                       evm_reset_cache_status(file_dentry(file), iint);
>                 }
>         }

Make sense.
Unrelated to your change, I now noticed something odd about Mimi's change:

        backing_inode = d_real_inode(file_dentry(file));

I find the choice of variable name to be quite confusing, because ima/evm code
uses  d_backing_inode() all over the place and d_real_inode() !=
d_backing_inode().

First of all, there is never any reason to use d_backing_inode() and its name is
quite confusing in the first place, but it will be a big cleanup to
remove them all.

Suggest to rename the variable to real_inode, same as in
ima_collect_measurement()
to be consistent and reduce confusion factor, which is already high enough ;)

Thanks,
Amir.

Re: [PATCH 5/5] evm: Enforce signatures on unsupported filesystem for EVM_INIT_X509

[Amir Goldstein]

On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> Unsupported filesystems currently do not enforce any signatures. Add
> support for signature enforcement of the "original" and "portable &
> immutable" signatures when EVM_INIT_X509 is enabled.
>
> The "original" signature type contains filesystem specific metadata.
> Thus it cannot be copied up and verified. However with EVM_INIT_X509
> and EVM_ALLOW_METADATA_WRITES enabled, the "original" file signature
> may be written.
>
> When EVM_ALLOW_METADATA_WRITES is not set or once it is removed from
> /sys/kernel/security/evm by setting EVM_INIT_HMAC for example, it is not
> possible to write or remove xattrs on the overlay filesystem.
>
> This change still prevents EVM from writing HMAC signatures on
> unsupported filesystem when EVM_INIT_HMAC is enabled.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  security/integrity/evm/evm_main.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index e96d127b48a2..f49609dfcbc7 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -192,7 +192,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>                      iint->evm_status == INTEGRITY_PASS_IMMUTABLE))
>                 return iint->evm_status;
>
> -       if (is_unsupported_fs(dentry))
> +       /*
> +        * On unsupported filesystems with EVM_INIT_X509 not enabled, skip
> +        * signature verification.
> +        */
> +       if (!(evm_initialized & EVM_INIT_X509) && is_unsupported_fs(dentry))
>                 return INTEGRITY_UNKNOWN;
>

Are the names is_unsupported_fs() and SB_I_EVM_UNSUPPORTED still
a good description of what overlayfs is after this change?
Is EVM really not supported on overlayfs after this change?

Would you consider a better descriptive name, for the helper and flag,
at least as descriptive as SB_I_IMA_UNVERIFIABLE_SIGNATURE?

Thanks,
Amir.

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Christian Brauner]

On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> > Copying up xattrs is solely based on the security xattr name. For finer
> > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > hook definition, allowing decisions to be based on the xattr content as
> > well.
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  fs/overlayfs/copy_up.c            | 2 +-
> >  include/linux/evm.h               | 2 +-
> >  include/linux/lsm_hook_defs.h     | 3 ++-
> >  include/linux/security.h          | 4 ++--
> >  security/integrity/evm/evm_main.c | 2 +-
> >  security/security.c               | 7 ++++---
> >  security/selinux/hooks.c          | 2 +-
> >  security/smack/smack_lsm.c        | 2 +-
> >  8 files changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index b8e25ca51016..bd9ddcefb7a7 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> >                 if (ovl_is_private_xattr(sb, name))
> >                         continue;
> >
> > -               error = security_inode_copy_up_xattr(name);
> > +               error = security_inode_copy_up_xattr(old, name);
> 
> What do you think about:
> 
>                      error = security_inode_copy_up_xattr(name, NULL, 0);
> 
> and then later...
> 
>                      error = security_inode_copy_up_xattr(name, value, size);
> 
> I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> have used nop_mnt_idmap inside evm hook.
> this does not look right to me?

So it's relevant if they interact with xattrs that care about the
idmapping and that's POSIX ACLs and fscaps. And only if they perform
permission checks such as posix_acl_update_mode() or something. IOW, it
depends on what exactly EVM is doing.

IIRC, I already added custom security_*() hooks specifically for POSIX
ACLs as they can't be retrieved through vfs_xattr*() helpers anymore.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 1/31/24 08:16, Amir Goldstein wrote:
> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 1/30/24 16:46, Stefan Berger wrote:
>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
>>> are not take into account when d_backing_inode() is used when a file is
>>> accessed on the overlay layer and this file has not yet been copied up.
>>> This is because d_backing_inode() does not return the real inode of the
>>> lower layer but instead returns the backing inode which holds old file
>>> attributes. When the old file attributes are used for calculating the
>>> metadata hash then the expected hash is calculated and the file then
>>> mistakenly passes signature verification. Therefore, use d_real_inode()
>>> which returns the inode of the lower layer for as long as the file has
>>> not been copied up and returns the upper layer's inode otherwise.
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>    security/integrity/evm/evm_crypto.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>> index b1ffd4cc0b44..2e48fe54e899 100644
>>> --- a/security/integrity/evm/evm_crypto.c
>>> +++ b/security/integrity/evm/evm_crypto.c
>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>>                                 size_t req_xattr_value_len,
>>>                                 uint8_t type, struct evm_digest *data)
>>>    {
>>> -     struct inode *inode = d_backing_inode(dentry);
>>> +     struct inode *inode = d_real_inode(dentry);
>>>        struct xattr_list *xattr;
>>>        struct shash_desc *desc;
>>>        size_t xattr_size = 0;
>>
>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
>> not sure what the solution is.
> 
> I think d_real_inode() does not work correctly for all its current users for
> a metacopy file.
> 
> I think the solution is to change d_real_inode() to return the data inode
> and add another helper to get the metadata inode if needed.
> I will post some patches for it.

I thought that we may have to go through vfs_getattr() but even better 
if we don't because we don't have the file *file anywhere 'near'.

> 
> However, I must say that I do not know if evm_calc_hmac_or_hash()
> needs the lower data inode, the upper metadata inode or both.

What it needs are data structures with mode bits, uid, and gid that stat 
in userspace would show.


> 
> The last time you tried to fix ovl+IMA, I asked for documentation
> of what data/metadata is protected with EVM and how are those
> protections supposed to work across overlayfs copy up, when the
> data and metadata are often split between 2 and myabe event 3
> differnt inode.

I always compare against what userspace sees with stat and that's what 
the EVM should also work with so it ends up in reasonable matching 
result in terms of hash calculation and then access permission/rejection.

> 
>  From the current patch set, I still don't understand what is the expected
> behavior before and after copy up of data/metadata-only.
> 
> Thanks,
> Amir.

Re: [PATCH 3/5] ima: Reset EVM status upon detecting changes to overlay backing file

[Stefan Berger]

On 1/31/24 08:56, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> To avoid caching effects to take effect reset the EVM status upon
>> detecting changes to the overlay backing files. This prevents a not-yet-
>> copied-up file on the overlay from executing if for example the
>> security.evm xattr on the file on the 'lower' layer has been removed.
>>
> 
> And what is expected to happen when file is executed after copy up?

The copy-up may be triggered by changing file content or file metadata.
For EVM file metadata (file attributes and xattrs) are important and if 
they change EVM would re-evaluate the file, meaning that it would 
determine the file mode bits, uid, gid and xattrs and calculate a hash 
over them and compare this hash against the signature in security.evm.

> Doesn't this change also protect the same threat after copy up?

 From what I remember from my testing is that file attribute or extended 
attribute changes on an already copied-up file were already handled 
correctly, meaning they caused the re-evaluation of the file as 
described above.

> 
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   include/linux/evm.h               | 8 ++++++++
>>   security/integrity/evm/evm_main.c | 7 +++++++
>>   security/integrity/ima/ima_main.c | 2 ++
>>   3 files changed, 17 insertions(+)
>>
>> diff --git a/include/linux/evm.h b/include/linux/evm.h
>> index d8c0343436b8..e7d6742eee9d 100644
>> --- a/include/linux/evm.h
>> +++ b/include/linux/evm.h
>> @@ -66,6 +66,8 @@ extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
>>   extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>>                                       int buffer_size, char type,
>>                                       bool canonical_fmt);
>> +extern void evm_reset_cache_status(struct dentry *dentry,
>> +                                  struct integrity_iint_cache *iint);
>>   #ifdef CONFIG_FS_POSIX_ACL
>>   extern int posix_xattr_acl(const char *xattrname);
>>   #else
>> @@ -189,5 +191,11 @@ static inline int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>>          return -EOPNOTSUPP;
>>   }
>>
>> +static inline void evm_reset_cache_status(struct dentry *dentry,
>> +                                         struct integrity_iint_cache *iint)
>> +{
>> +       return;
>> +}
>> +
>>   #endif /* CONFIG_EVM */
>>   #endif /* LINUX_EVM_H */
>> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
>> index 22a5e26860ea..e96d127b48a2 100644
>> --- a/security/integrity/evm/evm_main.c
>> +++ b/security/integrity/evm/evm_main.c
>> @@ -721,6 +721,13 @@ static void evm_reset_status(struct inode *inode)
>>                  iint->evm_status = INTEGRITY_UNKNOWN;
>>   }
>>
>> +void evm_reset_cache_status(struct dentry *dentry,
>> +                           struct integrity_iint_cache *iint)
>> +{
>> +       if (d_real_inode(dentry) != d_backing_inode(dentry))
>> +               iint->evm_status = INTEGRITY_UNKNOWN;
>> +}
>> +
>>   /**
>>    * evm_revalidate_status - report whether EVM status re-validation is necessary
>>    * @xattr_name: pointer to the affected extended attribute name
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index cc1217ac2c6f..84bdc6e58329 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/ima.h>
>>   #include <linux/fs.h>
>>   #include <linux/iversion.h>
>> +#include <linux/evm.h>
>>
>>   #include "ima.h"
>>
>> @@ -295,6 +296,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>>                      !inode_eq_iversion(backing_inode, iint->version)) {
>>                          iint->flags &= ~IMA_DONE_MASK;
>>                          iint->measured_pcrs = 0;
>> +                       evm_reset_cache_status(file_dentry(file), iint);
>>                  }
>>          }
> 
> Make sense.
> Unrelated to your change, I now noticed something odd about Mimi's change:
> 
>          backing_inode = d_real_inode(file_dentry(file));
> 
> I find the choice of variable name to be quite confusing, because ima/evm code
> uses  d_backing_inode() all over the place and d_real_inode() !=
> d_backing_inode().
> 
> First of all, there is never any reason to use d_backing_inode() and its name is
> quite confusing in the first place, but it will be a big cleanup to
> remove them all.
> 
> Suggest to rename the variable to real_inode, same as in
> ima_collect_measurement()
> to be consistent and reduce confusion factor, which is already high enough ;)
> 
> Thanks,
> Amir.

Re: [PATCH 0/5] evm: Support signatures on stacked filesystem

[Stefan Berger]

On 1/31/24 08:18, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> EVM has recently been completely disabled on unsupported (e.g.,
>> overlayfs). This series now enables copy-up of "portable and immutable"
>> signatures on those filesystems and enables the enforcement of
>> "portable and immutable" as well as the "original" signatures on
>> previously unsupported filesystem when EVM is enabled with EVM_INIT_X509.
>> HMAC verification and generation remains disabled on those filesystems.
>>
> 
> I am missing a high level description of what is in those "portable
> and immutable"
> signatures and how those signatures remain valid across copy up.
> 

 From 2/5:
"Portable and immutable EVM signatures can be copied up by stacked file-
system since the metadata their signature covers does not include file-
system-specific data such as a file's inode number, generation, and UUID."

Instead, the signatures cover file metadata such as file mode bits, uid, 
and gid as well as xattrs, which can all be preserved unchanged across a 
copy-up.

Reference: 
https://elixir.bootlin.com/linux/v6.7.2/source/security/integrity/evm/evm_crypto.c#L169


> Thanks,
> Amir.
> 

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Stefan Berger]

On 1/31/24 09:25, Christian Brauner wrote:
> On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
>> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>
>>> Copying up xattrs is solely based on the security xattr name. For finer
>>> granularity add a dentry parameter to the security_inode_copy_up_xattr
>>> hook definition, allowing decisions to be based on the xattr content as
>>> well.
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   fs/overlayfs/copy_up.c            | 2 +-
>>>   include/linux/evm.h               | 2 +-
>>>   include/linux/lsm_hook_defs.h     | 3 ++-
>>>   include/linux/security.h          | 4 ++--
>>>   security/integrity/evm/evm_main.c | 2 +-
>>>   security/security.c               | 7 ++++---
>>>   security/selinux/hooks.c          | 2 +-
>>>   security/smack/smack_lsm.c        | 2 +-
>>>   8 files changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>> index b8e25ca51016..bd9ddcefb7a7 100644
>>> --- a/fs/overlayfs/copy_up.c
>>> +++ b/fs/overlayfs/copy_up.c
>>> @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
>>>                  if (ovl_is_private_xattr(sb, name))
>>>                          continue;
>>>
>>> -               error = security_inode_copy_up_xattr(name);
>>> +               error = security_inode_copy_up_xattr(old, name);
>>
>> What do you think about:
>>
>>                       error = security_inode_copy_up_xattr(name, NULL, 0);
>>
>> and then later...
>>
>>                       error = security_inode_copy_up_xattr(name, value, size);
>>
>> I am asking because overlayfs uses mnt_idmap(path->mnt) and you
>> have used nop_mnt_idmap inside evm hook.
>> this does not look right to me?
> 
> So it's relevant if they interact with xattrs that care about the
> idmapping and that's POSIX ACLs and fscaps. And only if they perform
> permission checks such as posix_acl_update_mode() or something. IOW, it
> depends on what exactly EVM is doing.

In 2/5 we are reading the value of security.evm to look at its contents.

> 
> IIRC, I already added custom security_*() hooks specifically for POSIX
> ACLs as they can't be retrieved through vfs_xattr*() helpers anymore.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 1/31/24 08:16, Amir Goldstein wrote:
> > On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>
> >>
> >>
> >> On 1/30/24 16:46, Stefan Berger wrote:
> >>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
> >>> are not take into account when d_backing_inode() is used when a file is
> >>> accessed on the overlay layer and this file has not yet been copied up.
> >>> This is because d_backing_inode() does not return the real inode of the
> >>> lower layer but instead returns the backing inode which holds old file
> >>> attributes. When the old file attributes are used for calculating the
> >>> metadata hash then the expected hash is calculated and the file then
> >>> mistakenly passes signature verification. Therefore, use d_real_inode()
> >>> which returns the inode of the lower layer for as long as the file has
> >>> not been copied up and returns the upper layer's inode otherwise.
> >>>
> >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> >>> ---
> >>>    security/integrity/evm/evm_crypto.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> >>> index b1ffd4cc0b44..2e48fe54e899 100644
> >>> --- a/security/integrity/evm/evm_crypto.c
> >>> +++ b/security/integrity/evm/evm_crypto.c
> >>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> >>>                                 size_t req_xattr_value_len,
> >>>                                 uint8_t type, struct evm_digest *data)
> >>>    {
> >>> -     struct inode *inode = d_backing_inode(dentry);
> >>> +     struct inode *inode = d_real_inode(dentry);
> >>>        struct xattr_list *xattr;
> >>>        struct shash_desc *desc;
> >>>        size_t xattr_size = 0;
> >>
> >> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
> >> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
> >> not sure what the solution is.
> >
> > I think d_real_inode() does not work correctly for all its current users for
> > a metacopy file.
> >
> > I think the solution is to change d_real_inode() to return the data inode
> > and add another helper to get the metadata inode if needed.
> > I will post some patches for it.
>
> I thought that we may have to go through vfs_getattr() but even better
> if we don't because we don't have the file *file anywhere 'near'.
>
> >
> > However, I must say that I do not know if evm_calc_hmac_or_hash()
> > needs the lower data inode, the upper metadata inode or both.
>
> What it needs are data structures with mode bits, uid, and gid that stat
> in userspace would show.
>
>

With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
are always taken from the upper most inode which is what d_real_inode()
currently returns, so I do not understand what the problem is.

> >
> > The last time you tried to fix ovl+IMA, I asked for documentation
> > of what data/metadata is protected with EVM and how are those
> > protections supposed to work across overlayfs copy up, when the
> > data and metadata are often split between 2 and myabe event 3
> > differnt inode.
>
> I always compare against what userspace sees with stat and that's what
> the EVM should also work with so it ends up in reasonable matching
> result in terms of hash calculation and then access permission/rejection.
>

I will need a lot more analysis information to be able to help you.
Exactly which setup, exactly which test, exactly which inode/dentry/file
objects are used and how they are accessed when things go wrong.

Thanks,
Amir.

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[kernel test robot]

Hi Stefan,

kernel test robot noticed the following build errors:

[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on pcmoore-selinux/next linus/master v6.8-rc2 next-20240131]
[cannot apply to mszeredi-vfs/overlayfs-next mszeredi-vfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/security-allow-finer-granularity-in-permitting-copy-up-of-security-xattrs/20240131-054854
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link:    https://lore.kernel.org/r/20240130214620.3155380-2-stefanb%40linux.ibm.com
patch subject: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs
config: i386-buildonly-randconfig-002-20240131 (https://download.01.org/0day-ci/archive/20240201/202402010014.MArAf4UB-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240201/202402010014.MArAf4UB-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402010014.MArAf4UB-lkp@intel.com/

All errors (new ones prefixed by >>):

>> security/security.c:2627:38: error: too many arguments to function call, expected single argument 'name', have 2 arguments
    2627 |         return evm_inode_copy_up_xattr(src, name);
         |                ~~~~~~~~~~~~~~~~~~~~~~~      ^~~~
   include/linux/evm.h:121:20: note: 'evm_inode_copy_up_xattr' declared here
     121 | static inline int  evm_inode_copy_up_xattr(const char *name)
         |                    ^                       ~~~~~~~~~~~~~~~~
   1 error generated.


vim +/name +2627 security/security.c

  2596	
  2597	/**
  2598	 * security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
  2599	 * @src: union dentry of copy-up file
  2600	 * @name: xattr name
  2601	 *
  2602	 * Filter the xattrs being copied up when a unioned file is copied up from a
  2603	 * lower layer to the union/overlay layer.   The caller is responsible for
  2604	 * reading and writing the xattrs, this hook is merely a filter.
  2605	 *
  2606	 * Return: Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP
  2607	 *         if the security module does not know about attribute, or a negative
  2608	 *         error code to abort the copy up.
  2609	 */
  2610	int security_inode_copy_up_xattr(struct dentry *src, const char *name)
  2611	{
  2612		struct security_hook_list *hp;
  2613		int rc;
  2614	
  2615		/*
  2616		 * The implementation can return 0 (accept the xattr), 1 (discard the
  2617		 * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
  2618		 * any other error code in case of an error.
  2619		 */
  2620		hlist_for_each_entry(hp,
  2621				     &security_hook_heads.inode_copy_up_xattr, list) {
  2622			rc = hp->hook.inode_copy_up_xattr(src, name);
  2623			if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
  2624				return rc;
  2625		}
  2626	
> 2627		return evm_inode_copy_up_xattr(src, name);
  2628	}
  2629	EXPORT_SYMBOL(security_inode_copy_up_xattr);
  2630	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> >
> >
> > On 1/31/24 08:16, Amir Goldstein wrote:
> > > On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > >>
> > >>
> > >>
> > >> On 1/30/24 16:46, Stefan Berger wrote:
> > >>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
> > >>> are not take into account when d_backing_inode() is used when a file is
> > >>> accessed on the overlay layer and this file has not yet been copied up.
> > >>> This is because d_backing_inode() does not return the real inode of the
> > >>> lower layer but instead returns the backing inode which holds old file
> > >>> attributes. When the old file attributes are used for calculating the
> > >>> metadata hash then the expected hash is calculated and the file then
> > >>> mistakenly passes signature verification. Therefore, use d_real_inode()
> > >>> which returns the inode of the lower layer for as long as the file has
> > >>> not been copied up and returns the upper layer's inode otherwise.
> > >>>
> > >>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > >>> ---
> > >>>    security/integrity/evm/evm_crypto.c | 2 +-
> > >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > >>> index b1ffd4cc0b44..2e48fe54e899 100644
> > >>> --- a/security/integrity/evm/evm_crypto.c
> > >>> +++ b/security/integrity/evm/evm_crypto.c
> > >>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> > >>>                                 size_t req_xattr_value_len,
> > >>>                                 uint8_t type, struct evm_digest *data)
> > >>>    {
> > >>> -     struct inode *inode = d_backing_inode(dentry);
> > >>> +     struct inode *inode = d_real_inode(dentry);
> > >>>        struct xattr_list *xattr;
> > >>>        struct shash_desc *desc;
> > >>>        size_t xattr_size = 0;
> > >>
> > >> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
> > >> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
> > >> not sure what the solution is.
> > >
> > > I think d_real_inode() does not work correctly for all its current users for
> > > a metacopy file.
> > >
> > > I think the solution is to change d_real_inode() to return the data inode
> > > and add another helper to get the metadata inode if needed.
> > > I will post some patches for it.
> >
> > I thought that we may have to go through vfs_getattr() but even better
> > if we don't because we don't have the file *file anywhere 'near'.
> >
> > >
> > > However, I must say that I do not know if evm_calc_hmac_or_hash()
> > > needs the lower data inode, the upper metadata inode or both.
> >
> > What it needs are data structures with mode bits, uid, and gid that stat
> > in userspace would show.
> >
> >
>
> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
> are always taken from the upper most inode which is what d_real_inode()
> currently returns, so I do not understand what the problem is.
>

No, I was wrong. It is the other way around.
d_real_inode() always returns the real data inode and you need the
upper most real inode.

You can try this:

 -     struct inode *inode = d_backing_inode(dentry);
+     struct inode *inode = d_inode(d_real(dentry, false));

With the changes in:

https://github.com/amir73il/linux/commits/overlayfs-devel/

Not thoroughly tested...

Thanks,
Amir.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 1/31/24 10:54, Amir Goldstein wrote:
> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 1/31/24 08:16, Amir Goldstein wrote:
>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/30/24 16:46, Stefan Berger wrote:
>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
>>>>> are not take into account when d_backing_inode() is used when a file is
>>>>> accessed on the overlay layer and this file has not yet been copied up.
>>>>> This is because d_backing_inode() does not return the real inode of the
>>>>> lower layer but instead returns the backing inode which holds old file
>>>>> attributes. When the old file attributes are used for calculating the
>>>>> metadata hash then the expected hash is calculated and the file then
>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
>>>>> which returns the inode of the lower layer for as long as the file has
>>>>> not been copied up and returns the upper layer's inode otherwise.
>>>>>
>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>> ---
>>>>>     security/integrity/evm/evm_crypto.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
>>>>> --- a/security/integrity/evm/evm_crypto.c
>>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>>>>                                  size_t req_xattr_value_len,
>>>>>                                  uint8_t type, struct evm_digest *data)
>>>>>     {
>>>>> -     struct inode *inode = d_backing_inode(dentry);
>>>>> +     struct inode *inode = d_real_inode(dentry);
>>>>>         struct xattr_list *xattr;
>>>>>         struct shash_desc *desc;
>>>>>         size_t xattr_size = 0;
>>>>
>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
>>>> not sure what the solution is.
>>>
>>> I think d_real_inode() does not work correctly for all its current users for
>>> a metacopy file.
>>>
>>> I think the solution is to change d_real_inode() to return the data inode
>>> and add another helper to get the metadata inode if needed.
>>> I will post some patches for it.
>>
>> I thought that we may have to go through vfs_getattr() but even better
>> if we don't because we don't have the file *file anywhere 'near'.
>>
>>>
>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
>>> needs the lower data inode, the upper metadata inode or both.
>>
>> What it needs are data structures with mode bits, uid, and gid that stat
>> in userspace would show.
>>
>>
> 
> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
> are always taken from the upper most inode which is what d_real_inode()
> currently returns, so I do not understand what the problem is.

I have testcases that work fine with this series when 
CONFIG_OVERLAY_FS_METACOPY is not active. Once I activate this then a 
test case that changes a file's gid on the overlay layer from 0 to '12' 
while causing a copy-up allows a file to execute even thugh it should 
not execute. The reason is because d_real_inode(dentry)->i_guid shows 
the '0' while d_backing_dentry(dentry)->i_guid shows '12'. User space 
stat also shows '12' as expected.


Just saw your other email, will try that now ...

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 1/31/24 12:23, Amir Goldstein wrote:
> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>
>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>
>>>
>>>
>>> On 1/31/24 08:16, Amir Goldstein wrote:
>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 1/30/24 16:46, Stefan Berger wrote:
>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
>>>>>> are not take into account when d_backing_inode() is used when a file is
>>>>>> accessed on the overlay layer and this file has not yet been copied up.
>>>>>> This is because d_backing_inode() does not return the real inode of the
>>>>>> lower layer but instead returns the backing inode which holds old file
>>>>>> attributes. When the old file attributes are used for calculating the
>>>>>> metadata hash then the expected hash is calculated and the file then
>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
>>>>>> which returns the inode of the lower layer for as long as the file has
>>>>>> not been copied up and returns the upper layer's inode otherwise.
>>>>>>
>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>>> ---
>>>>>>     security/integrity/evm/evm_crypto.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
>>>>>> --- a/security/integrity/evm/evm_crypto.c
>>>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>>>>>                                  size_t req_xattr_value_len,
>>>>>>                                  uint8_t type, struct evm_digest *data)
>>>>>>     {
>>>>>> -     struct inode *inode = d_backing_inode(dentry);
>>>>>> +     struct inode *inode = d_real_inode(dentry);
>>>>>>         struct xattr_list *xattr;
>>>>>>         struct shash_desc *desc;
>>>>>>         size_t xattr_size = 0;
>>>>>
>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
>>>>> not sure what the solution is.
>>>>
>>>> I think d_real_inode() does not work correctly for all its current users for
>>>> a metacopy file.
>>>>
>>>> I think the solution is to change d_real_inode() to return the data inode
>>>> and add another helper to get the metadata inode if needed.
>>>> I will post some patches for it.
>>>
>>> I thought that we may have to go through vfs_getattr() but even better
>>> if we don't because we don't have the file *file anywhere 'near'.
>>>
>>>>
>>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
>>>> needs the lower data inode, the upper metadata inode or both.
>>>
>>> What it needs are data structures with mode bits, uid, and gid that stat
>>> in userspace would show.
>>>
>>>
>>
>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
>> are always taken from the upper most inode which is what d_real_inode()
>> currently returns, so I do not understand what the problem is.
>>
> 
> No, I was wrong. It is the other way around.
> d_real_inode() always returns the real data inode and you need the
> upper most real inode.
> 
> You can try this:
> 
>   -     struct inode *inode = d_backing_inode(dentry);
> +     struct inode *inode = d_inode(d_real(dentry, false));
> 
> With the changes in:
> 
> https://github.com/amir73il/linux/commits/overlayfs-devel/
> 
> Not thoroughly tested...

The change + 3 topmost patches cherry-picked is unfortunately are 
crashing for me.

FYI - in case you are interested: My tests running this with UML are here:

repo: https://github.com/stefanberger/ima-namespaces-tests.git
branch: overlayfs

There's a UML config in config/config.uml
Compiler kernel with this series applied: make ARCH=um -j128  && yes | 
cp linux /usr/local/bin/linux

sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 
evm+overlayfs-1/test.sh
sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 
evm+overlayfs-2/test.sh
sudo IMA_TEST_UML=/usr/local/bin/linux IMA_TEST_VERBOSE=0 
evm+overlayfs-3/test.sh

The 2nd and 3rd test case will fail at some point when metacopy is 
enabled, otherwise they will all pass.

> 
> Thanks,
> Amir.
> 

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[kernel test robot]

Hi Stefan,

kernel test robot noticed the following build errors:

[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on pcmoore-selinux/next linus/master v6.8-rc2 next-20240131]
[cannot apply to mszeredi-vfs/overlayfs-next mszeredi-vfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/security-allow-finer-granularity-in-permitting-copy-up-of-security-xattrs/20240131-054854
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link:    https://lore.kernel.org/r/20240130214620.3155380-2-stefanb%40linux.ibm.com
patch subject: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240201/202402010225.BXp3LrvU-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240201/202402010225.BXp3LrvU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402010225.BXp3LrvU-lkp@intel.com/

All errors (new ones prefixed by >>):

   security/security.c: In function 'security_inode_copy_up_xattr':
>> security/security.c:2627:40: error: passing argument 1 of 'evm_inode_copy_up_xattr' from incompatible pointer type [-Werror=incompatible-pointer-types]
    2627 |         return evm_inode_copy_up_xattr(src, name);
         |                                        ^~~
         |                                        |
         |                                        struct dentry *
   In file included from security/security.c:24:
   include/linux/evm.h:121:56: note: expected 'const char *' but argument is of type 'struct dentry *'
     121 | static inline int  evm_inode_copy_up_xattr(const char *name)
         |                                            ~~~~~~~~~~~~^~~~
>> security/security.c:2627:16: error: too many arguments to function 'evm_inode_copy_up_xattr'
    2627 |         return evm_inode_copy_up_xattr(src, name);
         |                ^~~~~~~~~~~~~~~~~~~~~~~
   In file included from security/security.c:24:
   include/linux/evm.h:121:20: note: declared here
     121 | static inline int  evm_inode_copy_up_xattr(const char *name)
         |                    ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/evm_inode_copy_up_xattr +2627 security/security.c

  2596	
  2597	/**
  2598	 * security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
  2599	 * @src: union dentry of copy-up file
  2600	 * @name: xattr name
  2601	 *
  2602	 * Filter the xattrs being copied up when a unioned file is copied up from a
  2603	 * lower layer to the union/overlay layer.   The caller is responsible for
  2604	 * reading and writing the xattrs, this hook is merely a filter.
  2605	 *
  2606	 * Return: Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP
  2607	 *         if the security module does not know about attribute, or a negative
  2608	 *         error code to abort the copy up.
  2609	 */
  2610	int security_inode_copy_up_xattr(struct dentry *src, const char *name)
  2611	{
  2612		struct security_hook_list *hp;
  2613		int rc;
  2614	
  2615		/*
  2616		 * The implementation can return 0 (accept the xattr), 1 (discard the
  2617		 * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
  2618		 * any other error code in case of an error.
  2619		 */
  2620		hlist_for_each_entry(hp,
  2621				     &security_hook_heads.inode_copy_up_xattr, list) {
  2622			rc = hp->hook.inode_copy_up_xattr(src, name);
  2623			if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
  2624				return rc;
  2625		}
  2626	
> 2627		return evm_inode_copy_up_xattr(src, name);
  2628	}
  2629	EXPORT_SYMBOL(security_inode_copy_up_xattr);
  2630	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 1/31/24 12:23, Amir Goldstein wrote:
> > On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >>
> >> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>
> >>>
> >>>
> >>> On 1/31/24 08:16, Amir Goldstein wrote:
> >>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 1/30/24 16:46, Stefan Berger wrote:
> >>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
> >>>>>> are not take into account when d_backing_inode() is used when a file is
> >>>>>> accessed on the overlay layer and this file has not yet been copied up.
> >>>>>> This is because d_backing_inode() does not return the real inode of the
> >>>>>> lower layer but instead returns the backing inode which holds old file
> >>>>>> attributes. When the old file attributes are used for calculating the
> >>>>>> metadata hash then the expected hash is calculated and the file then
> >>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
> >>>>>> which returns the inode of the lower layer for as long as the file has
> >>>>>> not been copied up and returns the upper layer's inode otherwise.
> >>>>>>
> >>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> >>>>>> ---
> >>>>>>     security/integrity/evm/evm_crypto.c | 2 +-
> >>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> >>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
> >>>>>> --- a/security/integrity/evm/evm_crypto.c
> >>>>>> +++ b/security/integrity/evm/evm_crypto.c
> >>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> >>>>>>                                  size_t req_xattr_value_len,
> >>>>>>                                  uint8_t type, struct evm_digest *data)
> >>>>>>     {
> >>>>>> -     struct inode *inode = d_backing_inode(dentry);
> >>>>>> +     struct inode *inode = d_real_inode(dentry);
> >>>>>>         struct xattr_list *xattr;
> >>>>>>         struct shash_desc *desc;
> >>>>>>         size_t xattr_size = 0;
> >>>>>
> >>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
> >>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
> >>>>> not sure what the solution is.
> >>>>
> >>>> I think d_real_inode() does not work correctly for all its current users for
> >>>> a metacopy file.
> >>>>
> >>>> I think the solution is to change d_real_inode() to return the data inode
> >>>> and add another helper to get the metadata inode if needed.
> >>>> I will post some patches for it.
> >>>
> >>> I thought that we may have to go through vfs_getattr() but even better
> >>> if we don't because we don't have the file *file anywhere 'near'.
> >>>
> >>>>
> >>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
> >>>> needs the lower data inode, the upper metadata inode or both.
> >>>
> >>> What it needs are data structures with mode bits, uid, and gid that stat
> >>> in userspace would show.
> >>>
> >>>
> >>
> >> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
> >> are always taken from the upper most inode which is what d_real_inode()
> >> currently returns, so I do not understand what the problem is.
> >>
> >
> > No, I was wrong. It is the other way around.
> > d_real_inode() always returns the real data inode and you need the
> > upper most real inode.
> >
> > You can try this:
> >
> >   -     struct inode *inode = d_backing_inode(dentry);
> > +     struct inode *inode = d_inode(d_real(dentry, false));
> >
> > With the changes in:
> >
> > https://github.com/amir73il/linux/commits/overlayfs-devel/
> >
> > Not thoroughly tested...
>
> The change + 3 topmost patches cherry-picked is unfortunately are
> crashing for me.
>

I will look into it.
But anyway, the patch I suggested above is not enough exactly because
of the reason I told you earlier.

Mimi's fix ("ima: detect changes to the backing overlay file") detects
a change in d_real_inode(file_dentry(file)) in order to invalidate the
IMA cache.

Your change also invalidates EVM cache on a change in
d_real_inode(file_dentry(file)) and that makes sense.

But on "meta copy up" for example on chmod(), an upper inode with no data
is created (a metacopy) and all the attributes and xattr are copied
from lower inode.
The data remains in the lower inode.

At this point , the IMA cache and the EVM cache refer to two different inodes
so you cannot share the same logic with IMA cache invalidation.

My patches are meant to provide you with a helper e.g. d_real_meta_inode()
that you could use to get the upper inode in the case of a metacopy, but
IMA would still need to use the d_real_data_inode().

Is that explanation clear? Is it clear why I said that the problem is more
complicated?

Thanks,
Amir.

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Christian Brauner]

On Wed, Jan 31, 2024 at 09:56:25AM -0500, Stefan Berger wrote:
> 
> 
> On 1/31/24 09:25, Christian Brauner wrote:
> > On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> > > On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > 
> > > > Copying up xattrs is solely based on the security xattr name. For finer
> > > > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > > > hook definition, allowing decisions to be based on the xattr content as
> > > > well.
> > > > 
> > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > ---
> > > >   fs/overlayfs/copy_up.c            | 2 +-
> > > >   include/linux/evm.h               | 2 +-
> > > >   include/linux/lsm_hook_defs.h     | 3 ++-
> > > >   include/linux/security.h          | 4 ++--
> > > >   security/integrity/evm/evm_main.c | 2 +-
> > > >   security/security.c               | 7 ++++---
> > > >   security/selinux/hooks.c          | 2 +-
> > > >   security/smack/smack_lsm.c        | 2 +-
> > > >   8 files changed, 13 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > index b8e25ca51016..bd9ddcefb7a7 100644
> > > > --- a/fs/overlayfs/copy_up.c
> > > > +++ b/fs/overlayfs/copy_up.c
> > > > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> > > >                  if (ovl_is_private_xattr(sb, name))
> > > >                          continue;
> > > > 
> > > > -               error = security_inode_copy_up_xattr(name);
> > > > +               error = security_inode_copy_up_xattr(old, name);
> > > 
> > > What do you think about:
> > > 
> > >                       error = security_inode_copy_up_xattr(name, NULL, 0);
> > > 
> > > and then later...
> > > 
> > >                       error = security_inode_copy_up_xattr(name, value, size);
> > > 
> > > I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> > > have used nop_mnt_idmap inside evm hook.
> > > this does not look right to me?
> > 
> > So it's relevant if they interact with xattrs that care about the
> > idmapping and that's POSIX ACLs and fscaps. And only if they perform
> > permission checks such as posix_acl_update_mode() or something. IOW, it
> > depends on what exactly EVM is doing.
> 
> In 2/5 we are reading the value of security.evm to look at its contents.

I'm not sure what this is supposed to be telling me in relation to the
original question though. :) security.evm doesn't store any {g,u}id
information afaict. IOW, it shouldn't matter?

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 2/1/24 07:10, Amir Goldstein wrote:
> On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 1/31/24 12:23, Amir Goldstein wrote:
>>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>>>
>>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 1/31/24 08:16, Amir Goldstein wrote:
>>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 1/30/24 16:46, Stefan Berger wrote:
>>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
>>>>>>>> are not take into account when d_backing_inode() is used when a file is
>>>>>>>> accessed on the overlay layer and this file has not yet been copied up.
>>>>>>>> This is because d_backing_inode() does not return the real inode of the
>>>>>>>> lower layer but instead returns the backing inode which holds old file
>>>>>>>> attributes. When the old file attributes are used for calculating the
>>>>>>>> metadata hash then the expected hash is calculated and the file then
>>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
>>>>>>>> which returns the inode of the lower layer for as long as the file has
>>>>>>>> not been copied up and returns the upper layer's inode otherwise.
>>>>>>>>
>>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>>>>> ---
>>>>>>>>      security/integrity/evm/evm_crypto.c | 2 +-
>>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
>>>>>>>> --- a/security/integrity/evm/evm_crypto.c
>>>>>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>>>>>>>                                   size_t req_xattr_value_len,
>>>>>>>>                                   uint8_t type, struct evm_digest *data)
>>>>>>>>      {
>>>>>>>> -     struct inode *inode = d_backing_inode(dentry);
>>>>>>>> +     struct inode *inode = d_real_inode(dentry);
>>>>>>>>          struct xattr_list *xattr;
>>>>>>>>          struct shash_desc *desc;
>>>>>>>>          size_t xattr_size = 0;
>>>>>>>
>>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
>>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
>>>>>>> not sure what the solution is.
>>>>>>
>>>>>> I think d_real_inode() does not work correctly for all its current users for
>>>>>> a metacopy file.
>>>>>>
>>>>>> I think the solution is to change d_real_inode() to return the data inode
>>>>>> and add another helper to get the metadata inode if needed.
>>>>>> I will post some patches for it.
>>>>>
>>>>> I thought that we may have to go through vfs_getattr() but even better
>>>>> if we don't because we don't have the file *file anywhere 'near'.
>>>>>
>>>>>>
>>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
>>>>>> needs the lower data inode, the upper metadata inode or both.
>>>>>
>>>>> What it needs are data structures with mode bits, uid, and gid that stat
>>>>> in userspace would show.
>>>>>
>>>>>
>>>>
>>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
>>>> are always taken from the upper most inode which is what d_real_inode()
>>>> currently returns, so I do not understand what the problem is.
>>>>
>>>
>>> No, I was wrong. It is the other way around.
>>> d_real_inode() always returns the real data inode and you need the
>>> upper most real inode.
>>>
>>> You can try this:
>>>
>>>    -     struct inode *inode = d_backing_inode(dentry);
>>> +     struct inode *inode = d_inode(d_real(dentry, false));
>>>
>>> With the changes in:
>>>
>>> https://github.com/amir73il/linux/commits/overlayfs-devel/
>>>
>>> Not thoroughly tested...
>>
>> The change + 3 topmost patches cherry-picked is unfortunately are
>> crashing for me.
>>
> 
> I will look into it.
> But anyway, the patch I suggested above is not enough exactly because
> of the reason I told you earlier.
> 
> Mimi's fix ("ima: detect changes to the backing overlay file") detects
> a change in d_real_inode(file_dentry(file)) in order to invalidate the
> IMA cache.
> 
> Your change also invalidates EVM cache on a change in
> d_real_inode(file_dentry(file)) and that makes sense.
> 
> But on "meta copy up" for example on chmod(), an upper inode with no data
> is created (a metacopy) and all the attributes and xattr are copied
> from lower inode.
> The data remains in the lower inode.
> 
> At this point , the IMA cache and the EVM cache refer to two different inodes

You mean they refer to different inodes because IMA cares about file 
content ("data remains in the lower inode:) and EVM cares about the 
metadata ("an upper inode with no data is created")? If so, I agree 
since the following line after copy-up with meatacopy enabled shows the 
proper GID is in the backing inode not the one return from 
d_real_inode(). If we knew that a meta copy has been done we could call 
d_backing_inode() in this case for access to mode bits, uid, and gid.

+       printk(KERN_INFO "real: GID: %d  backing: GID: %d\n",
+              from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid),
+              from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid));
+

 > so you cannot share the same logic with IMA cache invalidation.

I thought we we would have to share the same logic since IMA and EVM 
would have to refer to the same inode also since IMA and EVM are 
strongly connected. So the file_inode(file), which is typically used for 
finding the iint, should be the same 'high level' inode for both EVM and 
IMA I thought. A different inode could then be used for file data and 
metadata.

> 
> My patches are meant to provide you with a helper e.g. d_real_meta_inode()

The patch providing this isn't there yet in overlayfs-devel, right?

> that you could use to get the upper inode in the case of a metacopy, but
> IMA would still need to use the d_real_data_inode().

That would be fine since we are only changing EVM code in this case.
> 
> Is that explanation clear? Is it clear why I said that the problem is more
> complicated?

I think I understand it.

    Stefan

> 
> Thanks,
> Amir.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

On Thu, Feb 1, 2024 at 3:37 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 2/1/24 07:10, Amir Goldstein wrote:
> > On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>
> >>
> >>
> >> On 1/31/24 12:23, Amir Goldstein wrote:
> >>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >>>>
> >>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 1/31/24 08:16, Amir Goldstein wrote:
> >>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 1/30/24 16:46, Stefan Berger wrote:
> >>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
> >>>>>>>> are not take into account when d_backing_inode() is used when a file is
> >>>>>>>> accessed on the overlay layer and this file has not yet been copied up.
> >>>>>>>> This is because d_backing_inode() does not return the real inode of the
> >>>>>>>> lower layer but instead returns the backing inode which holds old file
> >>>>>>>> attributes. When the old file attributes are used for calculating the
> >>>>>>>> metadata hash then the expected hash is calculated and the file then
> >>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
> >>>>>>>> which returns the inode of the lower layer for as long as the file has
> >>>>>>>> not been copied up and returns the upper layer's inode otherwise.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> >>>>>>>> ---
> >>>>>>>>      security/integrity/evm/evm_crypto.c | 2 +-
> >>>>>>>>      1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> >>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
> >>>>>>>> --- a/security/integrity/evm/evm_crypto.c
> >>>>>>>> +++ b/security/integrity/evm/evm_crypto.c
> >>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> >>>>>>>>                                   size_t req_xattr_value_len,
> >>>>>>>>                                   uint8_t type, struct evm_digest *data)
> >>>>>>>>      {
> >>>>>>>> -     struct inode *inode = d_backing_inode(dentry);
> >>>>>>>> +     struct inode *inode = d_real_inode(dentry);
> >>>>>>>>          struct xattr_list *xattr;
> >>>>>>>>          struct shash_desc *desc;
> >>>>>>>>          size_t xattr_size = 0;
> >>>>>>>
> >>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
> >>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
> >>>>>>> not sure what the solution is.
> >>>>>>
> >>>>>> I think d_real_inode() does not work correctly for all its current users for
> >>>>>> a metacopy file.
> >>>>>>
> >>>>>> I think the solution is to change d_real_inode() to return the data inode
> >>>>>> and add another helper to get the metadata inode if needed.
> >>>>>> I will post some patches for it.
> >>>>>
> >>>>> I thought that we may have to go through vfs_getattr() but even better
> >>>>> if we don't because we don't have the file *file anywhere 'near'.
> >>>>>
> >>>>>>
> >>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
> >>>>>> needs the lower data inode, the upper metadata inode or both.
> >>>>>
> >>>>> What it needs are data structures with mode bits, uid, and gid that stat
> >>>>> in userspace would show.
> >>>>>
> >>>>>
> >>>>
> >>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
> >>>> are always taken from the upper most inode which is what d_real_inode()
> >>>> currently returns, so I do not understand what the problem is.
> >>>>
> >>>
> >>> No, I was wrong. It is the other way around.
> >>> d_real_inode() always returns the real data inode and you need the
> >>> upper most real inode.
> >>>
> >>> You can try this:
> >>>
> >>>    -     struct inode *inode = d_backing_inode(dentry);
> >>> +     struct inode *inode = d_inode(d_real(dentry, false));
> >>>
> >>> With the changes in:
> >>>
> >>> https://github.com/amir73il/linux/commits/overlayfs-devel/
> >>>
> >>> Not thoroughly tested...
> >>
> >> The change + 3 topmost patches cherry-picked is unfortunately are
> >> crashing for me.
> >>
> >
> > I will look into it.
> > But anyway, the patch I suggested above is not enough exactly because
> > of the reason I told you earlier.
> >
> > Mimi's fix ("ima: detect changes to the backing overlay file") detects
> > a change in d_real_inode(file_dentry(file)) in order to invalidate the
> > IMA cache.
> >
> > Your change also invalidates EVM cache on a change in
> > d_real_inode(file_dentry(file)) and that makes sense.
> >
> > But on "meta copy up" for example on chmod(), an upper inode with no data
> > is created (a metacopy) and all the attributes and xattr are copied
> > from lower inode.
> > The data remains in the lower inode.
> >
> > At this point , the IMA cache and the EVM cache refer to two different inodes
>
> You mean they refer to different inodes because IMA cares about file
> content ("data remains in the lower inode:) and EVM cares about the
> metadata ("an upper inode with no data is created")? If so, I agree

Correct.

> since the following line after copy-up with meatacopy enabled shows the
> proper GID is in the backing inode not the one return from
> d_real_inode(). If we knew that a meta copy has been done we could call
> d_backing_inode() in this case for access to mode bits, uid, and gid.
>

You should be able to use
d_real_meta_inode(dentry) != d_real_inode(dentry) to figure that out.

> +       printk(KERN_INFO "real: GID: %d  backing: GID: %d\n",
> +              from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid),
> +              from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid));
> +
>
>  > so you cannot share the same logic with IMA cache invalidation.
>
> I thought we we would have to share the same logic since IMA and EVM
> would have to refer to the same inode also since IMA and EVM are
> strongly connected. So the file_inode(file), which is typically used for
> finding the iint, should be the same 'high level' inode for both EVM and
> IMA I thought. A different inode could then be used for file data and
> metadata.
>
> >
> > My patches are meant to provide you with a helper e.g. d_real_meta_inode()
>
> The patch providing this isn't there yet in overlayfs-devel, right?

It's there just not spelled out with these helper names:

d_real_meta_inode(d) := d_inode(d_real(dentry, false))
d_real_data_inode(d) := d_inode(d_real(dentry, true))
d_real_inode(d) := d_real_data_inode(d)

I think this use case is pretty specific to EVM, so I don't think
I will actually define these d_real_*_inode() helpers.

>
> > that you could use to get the upper inode in the case of a metacopy, but
> > IMA would still need to use the d_real_data_inode().
>
> That would be fine since we are only changing EVM code in this case.

Yes, using those overlayfs APIs requires understanding of what they mean
and knowing how to test the affected use cases.
This is not something very common for other subsystem developers.

> >
> > Is that explanation clear? Is it clear why I said that the problem is more
> > complicated?
>
> I think I understand it.
>

I think I am also starting to understand the expectation from IMA/EVM
over overlayfs ;)

Anyway, let me see if I can fix the problem in my WIP branch.

Thanks,
Amir.

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Amir Goldstein]

On Thu, Feb 1, 2024 at 3:35 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Wed, Jan 31, 2024 at 09:56:25AM -0500, Stefan Berger wrote:
> >
> >
> > On 1/31/24 09:25, Christian Brauner wrote:
> > > On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> > > > On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > >
> > > > > Copying up xattrs is solely based on the security xattr name. For finer
> > > > > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > > > > hook definition, allowing decisions to be based on the xattr content as
> > > > > well.
> > > > >
> > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > > ---
> > > > >   fs/overlayfs/copy_up.c            | 2 +-
> > > > >   include/linux/evm.h               | 2 +-
> > > > >   include/linux/lsm_hook_defs.h     | 3 ++-
> > > > >   include/linux/security.h          | 4 ++--
> > > > >   security/integrity/evm/evm_main.c | 2 +-
> > > > >   security/security.c               | 7 ++++---
> > > > >   security/selinux/hooks.c          | 2 +-
> > > > >   security/smack/smack_lsm.c        | 2 +-
> > > > >   8 files changed, 13 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > > index b8e25ca51016..bd9ddcefb7a7 100644
> > > > > --- a/fs/overlayfs/copy_up.c
> > > > > +++ b/fs/overlayfs/copy_up.c
> > > > > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> > > > >                  if (ovl_is_private_xattr(sb, name))
> > > > >                          continue;
> > > > >
> > > > > -               error = security_inode_copy_up_xattr(name);
> > > > > +               error = security_inode_copy_up_xattr(old, name);
> > > >
> > > > What do you think about:
> > > >
> > > >                       error = security_inode_copy_up_xattr(name, NULL, 0);
> > > >
> > > > and then later...
> > > >
> > > >                       error = security_inode_copy_up_xattr(name, value, size);
> > > >
> > > > I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> > > > have used nop_mnt_idmap inside evm hook.
> > > > this does not look right to me?
> > >
> > > So it's relevant if they interact with xattrs that care about the
> > > idmapping and that's POSIX ACLs and fscaps. And only if they perform
> > > permission checks such as posix_acl_update_mode() or something. IOW, it
> > > depends on what exactly EVM is doing.
> >
> > In 2/5 we are reading the value of security.evm to look at its contents.
>
> I'm not sure what this is supposed to be telling me in relation to the
> original question though. :) security.evm doesn't store any {g,u}id
> information afaict. IOW, it shouldn't matter?

But it does. in evm_calc_hmac_or_hash() => hmac_add_misc():

        hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
        hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);

I guess as far as EVM is concerned, it should always be interested in the
absolute uig/gid values of the inode.

Thanks,
Amir.

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Stefan Berger]

On 1/31/24 08:25, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> Copying up xattrs is solely based on the security xattr name. For finer
>> granularity add a dentry parameter to the security_inode_copy_up_xattr
>> hook definition, allowing decisions to be based on the xattr content as
>> well.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   fs/overlayfs/copy_up.c            | 2 +-
>>   include/linux/evm.h               | 2 +-
>>   include/linux/lsm_hook_defs.h     | 3 ++-
>>   include/linux/security.h          | 4 ++--
>>   security/integrity/evm/evm_main.c | 2 +-
>>   security/security.c               | 7 ++++---
>>   security/selinux/hooks.c          | 2 +-
>>   security/smack/smack_lsm.c        | 2 +-
>>   8 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index b8e25ca51016..bd9ddcefb7a7 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
>>                  if (ovl_is_private_xattr(sb, name))
>>                          continue;
>>
>> -               error = security_inode_copy_up_xattr(name);
>> +               error = security_inode_copy_up_xattr(old, name);
> 
> What do you think about:
> 
>                       error = security_inode_copy_up_xattr(name, NULL, 0);

We need 'old'.
> 
> and then later...
> 
>                       error = security_inode_copy_up_xattr(name, value, size);

Are these parameter used to first query for the necessary size of the 
buffer and then provide the buffer to fill it? Or should the function 
rather take an existing buffer and realloc it if necessary and place the 
value of the xattr into it? Unfortunately this function currently 
returns '1' for 'discard', so returning the size of the xattr value from 
it maybe not ideal but it would require maybe yet another parameter that 
indicates what the size of the xattr value is.

    Stefan

> 
> I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> have used nop_mnt_idmap inside evm hook.
> this does not look right to me?
> 
> Thanks,
> Amir.

Re: [PATCH 5/5] evm: Enforce signatures on unsupported filesystem for EVM_INIT_X509

[Mimi Zohar]

On Wed, 2024-01-31 at 16:06 +0200, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com
> > wrote:
> > Unsupported filesystems currently do not enforce any signatures.
> > Add
> > support for signature enforcement of the "original" and "portable &
> > immutable" signatures when EVM_INIT_X509 is enabled.
> > 
> > The "original" signature type contains filesystem specific
> > metadata.
> > Thus it cannot be copied up and verified. However with
> > EVM_INIT_X509
> > and EVM_ALLOW_METADATA_WRITES enabled, the "original" file
> > signature
> > may be written.
> > 
> > When EVM_ALLOW_METADATA_WRITES is not set or once it is removed
> > from
> > /sys/kernel/security/evm by setting EVM_INIT_HMAC for example, it
> > is not
> > possible to write or remove xattrs on the overlay filesystem.
> > 
> > This change still prevents EVM from writing HMAC signatures on
> > unsupported filesystem when EVM_INIT_HMAC is enabled.
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  security/integrity/evm/evm_main.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > index e96d127b48a2..f49609dfcbc7 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -192,7 +192,11 @@ static enum integrity_status
> > evm_verify_hmac(struct dentry *dentry,
> >                      iint->evm_status == INTEGRITY_PASS_IMMUTABLE))
> >                 return iint->evm_status;
> > 
> > -       if (is_unsupported_fs(dentry))
> > +       /*
> > +        * On unsupported filesystems with EVM_INIT_X509 not
> > enabled, skip
> > +        * signature verification.
> > +        */
> > +       if (!(evm_initialized & EVM_INIT_X509) &&
> > is_unsupported_fs(dentry))
> >                 return INTEGRITY_UNKNOWN;
> > 
> 
> Are the names is_unsupported_fs() and SB_I_EVM_UNSUPPORTED still
> a good description of what overlayfs is after this change?
> Is EVM really not supported on overlayfs after this change?
> 
> Would you consider a better descriptive name, for the helper and
> flag,
> at least as descriptive as SB_I_IMA_UNVERIFIABLE_SIGNATURE?

The EVM "portable & immutable" signature can be copied up, because it
does not contain filesystem specific metadata.  Support for the
"original" EVM signature is limited, since it contains filesystem
specific metadata, but it could be used to sign the overlay filesystem
during a "setup" stage.

Like the "original" EVM signatues, the EVM HMAC contains filesystem
specific metadata.  For this reason, they too cannot be copied up.  

In addition, without first verifying the file's EVM HMAC on the lower
filesystem, calculating and writing the EVM HMAC on the overlay could
result in making the lower level file with an invalid HMAC, valid. 

SB_I_EVM_UNSUPPORTED could be renamed SB_I_EVM_HMAC_UNSUPPORTED.


Mimi

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 2/1/24 09:11, Amir Goldstein wrote:
> On Thu, Feb 1, 2024 at 3:37 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 2/1/24 07:10, Amir Goldstein wrote:
>>> On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/31/24 12:23, Amir Goldstein wrote:
>>>>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
>>>>>>
>>>>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 1/31/24 08:16, Amir Goldstein wrote:
>>>>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 1/30/24 16:46, Stefan Berger wrote:
>>>>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
>>>>>>>>>> are not take into account when d_backing_inode() is used when a file is
>>>>>>>>>> accessed on the overlay layer and this file has not yet been copied up.
>>>>>>>>>> This is because d_backing_inode() does not return the real inode of the
>>>>>>>>>> lower layer but instead returns the backing inode which holds old file
>>>>>>>>>> attributes. When the old file attributes are used for calculating the
>>>>>>>>>> metadata hash then the expected hash is calculated and the file then
>>>>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
>>>>>>>>>> which returns the inode of the lower layer for as long as the file has
>>>>>>>>>> not been copied up and returns the upper layer's inode otherwise.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>>>>>>>> ---
>>>>>>>>>>       security/integrity/evm/evm_crypto.c | 2 +-
>>>>>>>>>>       1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
>>>>>>>>>> --- a/security/integrity/evm/evm_crypto.c
>>>>>>>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
>>>>>>>>>>                                    size_t req_xattr_value_len,
>>>>>>>>>>                                    uint8_t type, struct evm_digest *data)
>>>>>>>>>>       {
>>>>>>>>>> -     struct inode *inode = d_backing_inode(dentry);
>>>>>>>>>> +     struct inode *inode = d_real_inode(dentry);
>>>>>>>>>>           struct xattr_list *xattr;
>>>>>>>>>>           struct shash_desc *desc;
>>>>>>>>>>           size_t xattr_size = 0;
>>>>>>>>>
>>>>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
>>>>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
>>>>>>>>> not sure what the solution is.
>>>>>>>>
>>>>>>>> I think d_real_inode() does not work correctly for all its current users for
>>>>>>>> a metacopy file.
>>>>>>>>
>>>>>>>> I think the solution is to change d_real_inode() to return the data inode
>>>>>>>> and add another helper to get the metadata inode if needed.
>>>>>>>> I will post some patches for it.
>>>>>>>
>>>>>>> I thought that we may have to go through vfs_getattr() but even better
>>>>>>> if we don't because we don't have the file *file anywhere 'near'.
>>>>>>>
>>>>>>>>
>>>>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
>>>>>>>> needs the lower data inode, the upper metadata inode or both.
>>>>>>>
>>>>>>> What it needs are data structures with mode bits, uid, and gid that stat
>>>>>>> in userspace would show.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
>>>>>> are always taken from the upper most inode which is what d_real_inode()
>>>>>> currently returns, so I do not understand what the problem is.
>>>>>>
>>>>>
>>>>> No, I was wrong. It is the other way around.
>>>>> d_real_inode() always returns the real data inode and you need the
>>>>> upper most real inode.
>>>>>
>>>>> You can try this:
>>>>>
>>>>>     -     struct inode *inode = d_backing_inode(dentry);
>>>>> +     struct inode *inode = d_inode(d_real(dentry, false));
>>>>>
>>>>> With the changes in:
>>>>>
>>>>> https://github.com/amir73il/linux/commits/overlayfs-devel/
>>>>>
>>>>> Not thoroughly tested...
>>>>
>>>> The change + 3 topmost patches cherry-picked is unfortunately are
>>>> crashing for me.
>>>>
>>>
>>> I will look into it.
>>> But anyway, the patch I suggested above is not enough exactly because
>>> of the reason I told you earlier.
>>>
>>> Mimi's fix ("ima: detect changes to the backing overlay file") detects
>>> a change in d_real_inode(file_dentry(file)) in order to invalidate the
>>> IMA cache.
>>>
>>> Your change also invalidates EVM cache on a change in
>>> d_real_inode(file_dentry(file)) and that makes sense.
>>>
>>> But on "meta copy up" for example on chmod(), an upper inode with no data
>>> is created (a metacopy) and all the attributes and xattr are copied
>>> from lower inode.
>>> The data remains in the lower inode.
>>>
>>> At this point , the IMA cache and the EVM cache refer to two different inodes
>>
>> You mean they refer to different inodes because IMA cares about file
>> content ("data remains in the lower inode:) and EVM cares about the
>> metadata ("an upper inode with no data is created")? If so, I agree
> 
> Correct.
> 
>> since the following line after copy-up with meatacopy enabled shows the
>> proper GID is in the backing inode not the one return from
>> d_real_inode(). If we knew that a meta copy has been done we could call
>> d_backing_inode() in this case for access to mode bits, uid, and gid.
>>
> 
> You should be able to use
> d_real_meta_inode(dentry) != d_real_inode(dentry) to figure that out.
> 
>> +       printk(KERN_INFO "real: GID: %d  backing: GID: %d\n",
>> +              from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid),
>> +              from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid));
>> +
>>
>>   > so you cannot share the same logic with IMA cache invalidation.
>>
>> I thought we we would have to share the same logic since IMA and EVM
>> would have to refer to the same inode also since IMA and EVM are
>> strongly connected. So the file_inode(file), which is typically used for
>> finding the iint, should be the same 'high level' inode for both EVM and
>> IMA I thought. A different inode could then be used for file data and
>> metadata.
>>
>>>
>>> My patches are meant to provide you with a helper e.g. d_real_meta_inode()
>>
>> The patch providing this isn't there yet in overlayfs-devel, right?
> 
> It's there just not spelled out with these helper names:
> 
> d_real_meta_inode(d) := d_inode(d_real(dentry, false))
> d_real_data_inode(d) := d_inode(d_real(dentry, true))
> d_real_inode(d) := d_real_data_inode(d)
> 
> I think this use case is pretty specific to EVM, so I don't think
> I will actually define these d_real_*_inode() helpers.
> 
>>
>>> that you could use to get the upper inode in the case of a metacopy, but
>>> IMA would still need to use the d_real_data_inode().
>>
>> That would be fine since we are only changing EVM code in this case.
> 
> Yes, using those overlayfs APIs requires understanding of what they mean
> and knowing how to test the affected use cases.
> This is not something very common for other subsystem developers.
> 
>>>
>>> Is that explanation clear? Is it clear why I said that the problem is more
>>> complicated?
>>
>> I think I understand it.
>>
> 
> I think I am also starting to understand the expectation from IMA/EVM
> over overlayfs ;)
> 
> Anyway, let me see if I can fix the problem in my WIP branch.

The good news is that with your two patches applied :

3b0ed3977dd2 (HEAD) fs: make file_dentry() a simple accessor
b5ccc40f3d50 fs: remove the inode argument to ->d_real() method

and your suggested change to this patch :

-       struct inode *inode = d_real_inode(dentry);
+       struct inode *inode = d_inode(d_real(dentry, false));;


The test cases are now passing with and without metacopy enabled. Yay!

Your 3rd patch causes crashes..

    Stefan


> 
> Thanks,
> Amir.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 2/1/24 09:11, Amir Goldstein wrote:
> > On Thu, Feb 1, 2024 at 3:37 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>
> >>
> >>
> >> On 2/1/24 07:10, Amir Goldstein wrote:
> >>> On Wed, Jan 31, 2024 at 7:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 1/31/24 12:23, Amir Goldstein wrote:
> >>>>> On Wed, Jan 31, 2024 at 5:54 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >>>>>>
> >>>>>> On Wed, Jan 31, 2024 at 4:40 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 1/31/24 08:16, Amir Goldstein wrote:
> >>>>>>>> On Wed, Jan 31, 2024 at 4:11 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On 1/30/24 16:46, Stefan Berger wrote:
> >>>>>>>>>> Changes to the file attribute (mode bits, uid, gid) on the lower layer
> >>>>>>>>>> are not take into account when d_backing_inode() is used when a file is
> >>>>>>>>>> accessed on the overlay layer and this file has not yet been copied up.
> >>>>>>>>>> This is because d_backing_inode() does not return the real inode of the
> >>>>>>>>>> lower layer but instead returns the backing inode which holds old file
> >>>>>>>>>> attributes. When the old file attributes are used for calculating the
> >>>>>>>>>> metadata hash then the expected hash is calculated and the file then
> >>>>>>>>>> mistakenly passes signature verification. Therefore, use d_real_inode()
> >>>>>>>>>> which returns the inode of the lower layer for as long as the file has
> >>>>>>>>>> not been copied up and returns the upper layer's inode otherwise.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> >>>>>>>>>> ---
> >>>>>>>>>>       security/integrity/evm/evm_crypto.c | 2 +-
> >>>>>>>>>>       1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> >>>>>>>>>> index b1ffd4cc0b44..2e48fe54e899 100644
> >>>>>>>>>> --- a/security/integrity/evm/evm_crypto.c
> >>>>>>>>>> +++ b/security/integrity/evm/evm_crypto.c
> >>>>>>>>>> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> >>>>>>>>>>                                    size_t req_xattr_value_len,
> >>>>>>>>>>                                    uint8_t type, struct evm_digest *data)
> >>>>>>>>>>       {
> >>>>>>>>>> -     struct inode *inode = d_backing_inode(dentry);
> >>>>>>>>>> +     struct inode *inode = d_real_inode(dentry);
> >>>>>>>>>>           struct xattr_list *xattr;
> >>>>>>>>>>           struct shash_desc *desc;
> >>>>>>>>>>           size_t xattr_size = 0;
> >>>>>>>>>
> >>>>>>>>> We need this patch when NOT activating CONFIG_OVERLAY_FS_METACOPY but
> >>>>>>>>> when setting CONFIG_OVERLAY_FS_METACOPY=y it has to be reverted...  I am
> >>>>>>>>> not sure what the solution is.
> >>>>>>>>
> >>>>>>>> I think d_real_inode() does not work correctly for all its current users for
> >>>>>>>> a metacopy file.
> >>>>>>>>
> >>>>>>>> I think the solution is to change d_real_inode() to return the data inode
> >>>>>>>> and add another helper to get the metadata inode if needed.
> >>>>>>>> I will post some patches for it.
> >>>>>>>
> >>>>>>> I thought that we may have to go through vfs_getattr() but even better
> >>>>>>> if we don't because we don't have the file *file anywhere 'near'.
> >>>>>>>
> >>>>>>>>
> >>>>>>>> However, I must say that I do not know if evm_calc_hmac_or_hash()
> >>>>>>>> needs the lower data inode, the upper metadata inode or both.
> >>>>>>>
> >>>>>>> What it needs are data structures with mode bits, uid, and gid that stat
> >>>>>>> in userspace would show.
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> With or without metacopy enabled, an overlay inode st_uid st_gid st_mode
> >>>>>> are always taken from the upper most inode which is what d_real_inode()
> >>>>>> currently returns, so I do not understand what the problem is.
> >>>>>>
> >>>>>
> >>>>> No, I was wrong. It is the other way around.
> >>>>> d_real_inode() always returns the real data inode and you need the
> >>>>> upper most real inode.
> >>>>>
> >>>>> You can try this:
> >>>>>
> >>>>>     -     struct inode *inode = d_backing_inode(dentry);
> >>>>> +     struct inode *inode = d_inode(d_real(dentry, false));
> >>>>>
> >>>>> With the changes in:
> >>>>>
> >>>>> https://github.com/amir73il/linux/commits/overlayfs-devel/
> >>>>>
> >>>>> Not thoroughly tested...
> >>>>
> >>>> The change + 3 topmost patches cherry-picked is unfortunately are
> >>>> crashing for me.
> >>>>
> >>>
> >>> I will look into it.
> >>> But anyway, the patch I suggested above is not enough exactly because
> >>> of the reason I told you earlier.
> >>>
> >>> Mimi's fix ("ima: detect changes to the backing overlay file") detects
> >>> a change in d_real_inode(file_dentry(file)) in order to invalidate the
> >>> IMA cache.
> >>>
> >>> Your change also invalidates EVM cache on a change in
> >>> d_real_inode(file_dentry(file)) and that makes sense.
> >>>
> >>> But on "meta copy up" for example on chmod(), an upper inode with no data
> >>> is created (a metacopy) and all the attributes and xattr are copied
> >>> from lower inode.
> >>> The data remains in the lower inode.
> >>>
> >>> At this point , the IMA cache and the EVM cache refer to two different inodes
> >>
> >> You mean they refer to different inodes because IMA cares about file
> >> content ("data remains in the lower inode:) and EVM cares about the
> >> metadata ("an upper inode with no data is created")? If so, I agree
> >
> > Correct.
> >
> >> since the following line after copy-up with meatacopy enabled shows the
> >> proper GID is in the backing inode not the one return from
> >> d_real_inode(). If we knew that a meta copy has been done we could call
> >> d_backing_inode() in this case for access to mode bits, uid, and gid.
> >>
> >
> > You should be able to use
> > d_real_meta_inode(dentry) != d_real_inode(dentry) to figure that out.
> >
> >> +       printk(KERN_INFO "real: GID: %d  backing: GID: %d\n",
> >> +              from_kgid(&init_user_ns, d_real_inode(dentry)->i_gid),
> >> +              from_kgid(&init_user_ns, d_backing_inode(dentry)->i_gid));
> >> +
> >>
> >>   > so you cannot share the same logic with IMA cache invalidation.
> >>
> >> I thought we we would have to share the same logic since IMA and EVM
> >> would have to refer to the same inode also since IMA and EVM are
> >> strongly connected. So the file_inode(file), which is typically used for
> >> finding the iint, should be the same 'high level' inode for both EVM and
> >> IMA I thought. A different inode could then be used for file data and
> >> metadata.
> >>
> >>>
> >>> My patches are meant to provide you with a helper e.g. d_real_meta_inode()
> >>
> >> The patch providing this isn't there yet in overlayfs-devel, right?
> >
> > It's there just not spelled out with these helper names:
> >
> > d_real_meta_inode(d) := d_inode(d_real(dentry, false))
> > d_real_data_inode(d) := d_inode(d_real(dentry, true))
> > d_real_inode(d) := d_real_data_inode(d)
> >
> > I think this use case is pretty specific to EVM, so I don't think
> > I will actually define these d_real_*_inode() helpers.
> >
> >>
> >>> that you could use to get the upper inode in the case of a metacopy, but
> >>> IMA would still need to use the d_real_data_inode().
> >>
> >> That would be fine since we are only changing EVM code in this case.
> >
> > Yes, using those overlayfs APIs requires understanding of what they mean
> > and knowing how to test the affected use cases.
> > This is not something very common for other subsystem developers.
> >
> >>>
> >>> Is that explanation clear? Is it clear why I said that the problem is more
> >>> complicated?
> >>
> >> I think I understand it.
> >>
> >
> > I think I am also starting to understand the expectation from IMA/EVM
> > over overlayfs ;)
> >
> > Anyway, let me see if I can fix the problem in my WIP branch.
>
> The good news is that with your two patches applied :
>
> 3b0ed3977dd2 (HEAD) fs: make file_dentry() a simple accessor
> b5ccc40f3d50 fs: remove the inode argument to ->d_real() method

Doh! good catch!
This first fix commit was buggy.

Pushed a new fixed version:

* 4d76c382bf12 - (github/overlayfs-devel) fs: remove the inode
argument to ->d_real() method
* 2cadd1b25485 - fs: make file_dentry() a simple accessor
* 1c5e7db8e1b2 - (github/ovl-fixes) remap_range: merge
do_clone_file_range() into vfs_clone_file_range()

>
> and your suggested change to this patch :
>
> -       struct inode *inode = d_real_inode(dentry);
> +       struct inode *inode = d_inode(d_real(dentry, false));;
>

In the new version I change the API to use an enum instead of bool, e.g.:

       struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));

This catches in build time and in run time, callers that were not converted
to the new API.

> The test cases are now passing with and without metacopy enabled. Yay!

Too soon to be happy.
I guess you are missing a test for the following case:
1. file was meta copied up (change is detected)
2. the lower file that contains the data is being changed (change is
not detected)

At #2 the change is not detected, because the inode version of the
lower inode is not checked.

I think the only way for you to cover this case is to store versions of both
real data and real metadata inodes...

Let me know if this version work for you and I will added you Tested-by
when I post it.

Thanks,
Amir.

Re: [PATCH 1/5] security: allow finer granularity in permitting copy-up of security xattrs

[Christian Brauner]

On Thu, Feb 01, 2024 at 04:18:32PM +0200, Amir Goldstein wrote:
> On Thu, Feb 1, 2024 at 3:35 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Wed, Jan 31, 2024 at 09:56:25AM -0500, Stefan Berger wrote:
> > >
> > >
> > > On 1/31/24 09:25, Christian Brauner wrote:
> > > > On Wed, Jan 31, 2024 at 03:25:29PM +0200, Amir Goldstein wrote:
> > > > > On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > > > >
> > > > > > Copying up xattrs is solely based on the security xattr name. For finer
> > > > > > granularity add a dentry parameter to the security_inode_copy_up_xattr
> > > > > > hook definition, allowing decisions to be based on the xattr content as
> > > > > > well.
> > > > > >
> > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > > > ---
> > > > > >   fs/overlayfs/copy_up.c            | 2 +-
> > > > > >   include/linux/evm.h               | 2 +-
> > > > > >   include/linux/lsm_hook_defs.h     | 3 ++-
> > > > > >   include/linux/security.h          | 4 ++--
> > > > > >   security/integrity/evm/evm_main.c | 2 +-
> > > > > >   security/security.c               | 7 ++++---
> > > > > >   security/selinux/hooks.c          | 2 +-
> > > > > >   security/smack/smack_lsm.c        | 2 +-
> > > > > >   8 files changed, 13 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > > > > index b8e25ca51016..bd9ddcefb7a7 100644
> > > > > > --- a/fs/overlayfs/copy_up.c
> > > > > > +++ b/fs/overlayfs/copy_up.c
> > > > > > @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> > > > > >                  if (ovl_is_private_xattr(sb, name))
> > > > > >                          continue;
> > > > > >
> > > > > > -               error = security_inode_copy_up_xattr(name);
> > > > > > +               error = security_inode_copy_up_xattr(old, name);
> > > > >
> > > > > What do you think about:
> > > > >
> > > > >                       error = security_inode_copy_up_xattr(name, NULL, 0);
> > > > >
> > > > > and then later...
> > > > >
> > > > >                       error = security_inode_copy_up_xattr(name, value, size);
> > > > >
> > > > > I am asking because overlayfs uses mnt_idmap(path->mnt) and you
> > > > > have used nop_mnt_idmap inside evm hook.
> > > > > this does not look right to me?
> > > >
> > > > So it's relevant if they interact with xattrs that care about the
> > > > idmapping and that's POSIX ACLs and fscaps. And only if they perform
> > > > permission checks such as posix_acl_update_mode() or something. IOW, it
> > > > depends on what exactly EVM is doing.
> > >
> > > In 2/5 we are reading the value of security.evm to look at its contents.
> >
> > I'm not sure what this is supposed to be telling me in relation to the
> > original question though. :) security.evm doesn't store any {g,u}id
> > information afaict. IOW, it shouldn't matter?
> 
> But it does. in evm_calc_hmac_or_hash() => hmac_add_misc():
> 
>         hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
>         hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
> 
> I guess as far as EVM is concerned, it should always be interested in the
> absolute uig/gid values of the inode.

There we go, thanks Amir. Yes, these EVM values can't be relative to any
idmappings. If inode->i_uid has kuid 100000 and 100000 maps to zero in
the caller's user namespace then you'd be storing hmac_misc.{g,u}id
zero. That would be problematic as it would give the impression that
real root caused that hmac to be written. So this really needs to store
100000 to make it clear that this was an unprivileged user that created
these values.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 2/2/24 04:24, Amir Goldstein wrote:
> On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote:

> 
>>
>> and your suggested change to this patch :
>>
>> -       struct inode *inode = d_real_inode(dentry);
>> +       struct inode *inode = d_inode(d_real(dentry, false));;
>>
> 
> In the new version I change the API to use an enum instead of bool, e.g.:
> 
>         struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));

Thanks. I will use it.

> 
> This catches in build time and in run time, callers that were not converted
> to the new API.
> 
>> The test cases are now passing with and without metacopy enabled. Yay!
> 
> Too soon to be happy.
> I guess you are missing a test for the following case:
> 1. file was meta copied up (change is detected)
> 2. the lower file that contains the data is being changed (change is
> not detected)

Right. Though it seems there's something wrong with overlayfs as well 
after appending a byte to the file on the lower.

-rwxr-xr-x    1 0        0               25 Feb  2 14:55 
/ext4.mount/lower/test_rsa_portable2
-rwxr-xr-x    1 0        0               24 Feb  2 14:55 
/ext4.mount/overlay/test_rsa_portable2
bb16aa5350bcc8863da1a873c846fec9281842d9 
/ext4.mount/lower/test_rsa_portable2
bb16aa5350bcc8863da1a873c846fec9281842d9 
/ext4.mount/overlay/test_rsa_portable2

We have a hash collision on a file with 24 bytes and the underlying one 
with 25 byte. (-;  :-)

   Stefan

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

On Fri, Feb 2, 2024 at 4:59 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 2/2/24 04:24, Amir Goldstein wrote:
> > On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> >
> >>
> >> and your suggested change to this patch :
> >>
> >> -       struct inode *inode = d_real_inode(dentry);
> >> +       struct inode *inode = d_inode(d_real(dentry, false));;
> >>
> >
> > In the new version I change the API to use an enum instead of bool, e.g.:
> >
> >         struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));
>
> Thanks. I will use it.
>
> >
> > This catches in build time and in run time, callers that were not converted
> > to the new API.
> >
> >> The test cases are now passing with and without metacopy enabled. Yay!
> >
> > Too soon to be happy.
> > I guess you are missing a test for the following case:
> > 1. file was meta copied up (change is detected)
> > 2. the lower file that contains the data is being changed (change is
> > not detected)
>
> Right. Though it seems there's something wrong with overlayfs as well
> after appending a byte to the file on the lower.
>
> -rwxr-xr-x    1 0        0               25 Feb  2 14:55
> /ext4.mount/lower/test_rsa_portable2
> -rwxr-xr-x    1 0        0               24 Feb  2 14:55
> /ext4.mount/overlay/test_rsa_portable2
> bb16aa5350bcc8863da1a873c846fec9281842d9
> /ext4.mount/lower/test_rsa_portable2
> bb16aa5350bcc8863da1a873c846fec9281842d9
> /ext4.mount/overlay/test_rsa_portable2
>
> We have a hash collision on a file with 24 bytes and the underlying one
> with 25 byte. (-;  :-)

https://docs.kernel.org/filesystems/overlayfs.html#changes-to-underlying-filesystems

If you modify the lower file underneath overlayfs, you get no
guarantee from overlayfs about expected results.

This makes your work more challenging.

Thanks,
Amir.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 2/2/24 10:51, Amir Goldstein wrote:
> On Fri, Feb 2, 2024 at 4:59 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 2/2/24 04:24, Amir Goldstein wrote:
>>> On Thu, Feb 1, 2024 at 10:35 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>>
>>>>
>>>> and your suggested change to this patch :
>>>>
>>>> -       struct inode *inode = d_real_inode(dentry);
>>>> +       struct inode *inode = d_inode(d_real(dentry, false));;
>>>>
>>>
>>> In the new version I change the API to use an enum instead of bool, e.g.:
>>>
>>>          struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));
>>
>> Thanks. I will use it.
>>
>>>
>>> This catches in build time and in run time, callers that were not converted
>>> to the new API.
>>>
>>>> The test cases are now passing with and without metacopy enabled. Yay!
>>>
>>> Too soon to be happy.
>>> I guess you are missing a test for the following case:
>>> 1. file was meta copied up (change is detected)
>>> 2. the lower file that contains the data is being changed (change is
>>> not detected)
>>
>> Right. Though it seems there's something wrong with overlayfs as well
>> after appending a byte to the file on the lower.
>>
>> -rwxr-xr-x    1 0        0               25 Feb  2 14:55
>> /ext4.mount/lower/test_rsa_portable2
>> -rwxr-xr-x    1 0        0               24 Feb  2 14:55
>> /ext4.mount/overlay/test_rsa_portable2
>> bb16aa5350bcc8863da1a873c846fec9281842d9
>> /ext4.mount/lower/test_rsa_portable2
>> bb16aa5350bcc8863da1a873c846fec9281842d9
>> /ext4.mount/overlay/test_rsa_portable2
>>
>> We have a hash collision on a file with 24 bytes and the underlying one
>> with 25 byte. (-;  :-)
> 
> https://docs.kernel.org/filesystems/overlayfs.html#changes-to-underlying-filesystems
> 
> If you modify the lower file underneath overlayfs, you get no
> guarantee from overlayfs about expected results.
> 
> This makes your work more challenging.
The odd thing is my updated test case '2' seems to indicate that 
everything already works as expected with CONFIG_OVERLAY_FS_METACOPY=y. 
After causing copy-up of metadata changes to the file content on the 
lower layer still cause permission error to file execution on the 
overlay layer and after restoring the file content on the lower the file 
on the overlay again runs as expected. The file content change + copy-up 
of file content also has completely decoupled the lower file from the 
file on the overlay and changes to the file on the lower cause no more 
file execution rejections on the overlay.

  Stefan
> 
> Thanks,
> Amir.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Amir Goldstein]

> The odd thing is my updated test case '2' seems to indicate that
> everything already works as expected with CONFIG_OVERLAY_FS_METACOPY=y.
> After causing copy-up of metadata changes to the file content on the
> lower layer still cause permission error to file execution on the
> overlay layer and after restoring the file content on the lower the file
> on the overlay again runs as expected. The file content change + copy-up
> of file content also has completely decoupled the lower file from the
> file on the overlay and changes to the file on the lower cause no more
> file execution rejections on the overlay.
>

Sorry, you lost me.
The combination of IMA+EVM+OVL must be too complicated to
explain in plain language without an explicit test spelled out...

When you write "The file content change + copy-up of file content also
has completely decoupled the lower file from the file on the overlay",
what do you mean by "copy up of the file content"?
Why was the file content copied up?
I was asking about use case that only metadata was copied up but
lower file content, which is still the content of the ovl file was changed
underneath ovl - this case does not cause data content to be copied up.

I don't think we understand each other.

Thanks,
Amir.

Re: [PATCH 4/5] evm: Use the real inode's metadata to calculate metadata hash

[Stefan Berger]

On 2/2/24 11:17, Amir Goldstein wrote:
>> The odd thing is my updated test case '2' seems to indicate that
>> everything already works as expected with CONFIG_OVERLAY_FS_METACOPY=y.
>> After causing copy-up of metadata changes to the file content on the
>> lower layer still cause permission error to file execution on the
>> overlay layer and after restoring the file content on the lower the file
>> on the overlay again runs as expected. The file content change + copy-up
>> of file content also has completely decoupled the lower file from the
>> file on the overlay and changes to the file on the lower cause no more
>> file execution rejections on the overlay.
>>
> 
> Sorry, you lost me.
> The combination of IMA+EVM+OVL must be too complicated to
> explain in plain language without an explicit test spelled out...
> 
> When you write "The file content change + copy-up of file content also
> has completely decoupled the lower file from the file on the overlay",
> what do you mean by "copy up of the file content"?
> Why was the file content copied up?

The file was copied up by appending a byte to the file on the 'overlay'.

> I was asking about use case that only metadata was copied up but
> lower file content, which is still the content of the ovl file was changed
> underneath ovl - this case does not cause data content to be copied up.
> 
> I don't think we understand each other.

One of the test cases I also have is appending a byte to the file on the
'lower'. At this point in the test one can detect whether 
CONFIG_OVERLAY_FS_METACOPY is enabled by checking the sha1 of the files 
on the lower and overlay layers and comparing their hashes. If they are 
equal then CONFIG_OVERLAY_FS_METACOPY is enabled since previously in the 
test file metadata on the overlay layer was already changed, which in 
the CONFIG_OVERLAY_FS_METACOPY=y case only caused a copy-up of metadata.
So, when trying to execute the file on the overlay layer the file cannot 
be executed due to the file content change on the lower layer (IMA 
should be the one detecting this, need to check) still 'shining 
through'. After restoring the file content on the lower layer the file 
again executes on the 'overlay' layer - as expected.

    Stefan


> 
> Thanks,
> Amir.