[PATCH] evm: Correct inode_init_security hooks behaviors

[PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>

Fixes a NULL pointer dereference occuring in the
`evm_protected_xattr_common` function of the EVM LSM. The bug is
triggered if a `inode_init_security` hook returns 0 without initializing
the given `struct xattr` fields (which is the case of BPF) and if no
other LSM overrides thoses fields after. This also leads to memory
leaks.

Adds a `call_int_hook_xattr` macro that fetches and feed the
`new_xattrs` array with every called hook xattr values.

Adds a `evm_init_hmacs` function which init the EVM hmac using every
entry of the array contrary to `evm_init_hmac`.

Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.

The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.

Changes the default return value of the `inode_init_security` hook
definition to `-EOPNOTSUPP`.

Changes the hook documentation to match the behavior of the LSMs using
it (only xattr->value is initialised with kmalloc and thus is the only
one that should be kfreed by the caller).

Cc: stable@vger.kernel.org
Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
---
 include/linux/lsm_hook_defs.h       |  2 +-
 include/linux/lsm_hooks.h           |  4 ++--
 security/integrity/evm/evm.h        |  2 ++
 security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
 security/integrity/evm/evm_main.c   | 11 ++++++-----
 security/security.c                 | 29 ++++++++++++++++++++++++++---
 6 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 806448173033..e5dd0c0f6345 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
 	 unsigned int obj_type)
 LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
 LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
-LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
+LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
 	 struct inode *dir, const struct qstr *qstr, const char **name,
 	 void **value, size_t *len)
 LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 84a0d7e02176..95aff9383de1 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -229,8 +229,8 @@
  *	This hook is called by the fs code as part of the inode creation
  *	transaction and provides for atomic labeling of the inode, unlike
  *	the post_create/mkdir/... hooks called by the VFS.  The hook function
- *	is expected to allocate the name and value via kmalloc, with the caller
- *	being responsible for calling kfree after using them.
+ *	is expected to allocate the value via kmalloc, with the caller
+ *	being responsible for calling kfree after using it.
  *	If the security module does not use security attributes or does
  *	not wish to put a security attribute on this particular inode,
  *	then it should return -EOPNOTSUPP to skip this processing.
diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
index f8b8c5004fc7..a2f9886e924d 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
 		  struct evm_digest *data);
 int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
 		  char *hmac_val);
+int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
+		  char *hmac_val);
 int evm_init_secfs(void);
 
 #endif
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 708de9656bbd..e5a34306cab6 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
 	return rc;
 }
 
-
 /*
  * Calculate the hmac and update security.evm xattr
  *
@@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 	return rc;
 }
 
+int evm_protected_xattr(const char *req_xattr_name);
+
+int evm_init_hmacs(struct inode *inode, const struct xattr *lsm_xattrs,
+		  char *hmac_val)
+{
+	struct shash_desc *desc;
+
+	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
+	if (IS_ERR(desc)) {
+		pr_info("init_desc failed\n");
+		return PTR_ERR(desc);
+	}
+
+	for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
+		if (evm_protected_xattr(lsm_xattrs[i].name))
+			crypto_shash_update(desc, lsm_xattrs[i].value, lsm_xattrs[i].value_len);
+	}
+	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
+	kfree(desc);
+	return 0;
+}
+
 int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 		  char *hmac_val)
 {
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2e6fb6e2ffd2..bb071c55d656 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
 	int found = 0;
 	struct xattr_list *xattr;
 
+	if (!req_xattr_name)
+		return found;
 	namelen = strlen(req_xattr_name);
 	list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) {
 		if (!all_xattrs && !xattr->enabled)
@@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
 	return found;
 }
 
-static int evm_protected_xattr(const char *req_xattr_name)
+int evm_protected_xattr(const char *req_xattr_name)
 {
 	return evm_protected_xattr_common(req_xattr_name, false);
 }
@@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
  * evm_inode_init_security - initializes security.evm HMAC value
  */
 int evm_inode_init_security(struct inode *inode,
-				 const struct xattr *lsm_xattr,
+				 const struct xattr *lsm_xattrs,
 				 struct xattr *evm_xattr)
 {
 	struct evm_xattr *xattr_data;
 	int rc;
 
-	if (!(evm_initialized & EVM_INIT_HMAC) ||
-	    !evm_protected_xattr(lsm_xattr->name))
+	if (!(evm_initialized & EVM_INIT_HMAC))
 		return 0;
 
 	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
@@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
 		return -ENOMEM;
 
 	xattr_data->data.type = EVM_XATTR_HMAC;
-	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
+	rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
 	if (rc < 0)
 		goto out;
 
diff --git a/security/security.c b/security/security.c
index 14d30fec8a00..47012c118536 100644
--- a/security/security.c
+++ b/security/security.c
@@ -30,7 +30,7 @@
 #include <linux/msg.h>
 #include <net/flow.h>
 
-#define MAX_LSM_EVM_XATTR	2
+#define MAX_LSM_EVM_XATTR	5
 
 /* How many LSMs were built into the kernel? */
 #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
@@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct super_block *sb)
 	RC;							\
 })
 
+#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({		\
+	int RC = IRC;						\
+	int i = 0;						\
+	do {							\
+		struct security_hook_list *P;			\
+								\
+		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
+			RC = P->hook.FUNC(__VA_ARGS__);		\
+			if (RC == -EOPNOTSUPP)			\
+				continue;			\
+			if (RC != 0 && RC != IRC)		\
+				break;				\
+			if (i >= MAX_LSM_EVM_XATTR) {		\
+				RC = -ENOMEM;			\
+				break;				\
+			}					\
+			XATTRS++;				\
+			i++;					\
+		}						\
+	} while (0);						\
+	RC;							\
+})
+
 /* Security operations */
 
 int security_binder_set_context_mgr(const struct cred *mgr)
@@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
 				     dir, qstr, NULL, NULL, NULL);
 	memset(new_xattrs, 0, sizeof(new_xattrs));
 	lsm_xattr = new_xattrs;
-	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
+	ret = call_int_hook_xattr(lsm_xattr, inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
 						&lsm_xattr->name,
 						&lsm_xattr->value,
 						&lsm_xattr->value_len);
@@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
 		goto out;
 
 	evm_xattr = lsm_xattr + 1;
-	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
+	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
 	if (ret)
 		goto out;
 	ret = initxattrs(inode, new_xattrs, fs_data);
-- 
2.38.1

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Paul Moore]

On Thu, Oct 20, 2022 at 9:55 AM Nicolas Bouchinet
<nicolas.bouchinet@clip-os.org> wrote:
>
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>
> Fixes a NULL pointer dereference occuring in the
> `evm_protected_xattr_common` function of the EVM LSM. The bug is
> triggered if a `inode_init_security` hook returns 0 without initializing
> the given `struct xattr` fields (which is the case of BPF) and if no
> other LSM overrides thoses fields after. This also leads to memory
> leaks.

You'll have to forgive me, my connection is poor at the moment and my
time is limited, but why not simply add some additional checking at
the top of evm_inode_init_security()? The LSM hook already memset()'s
the passed lsm_attrs to zero so xattr::{name,value,value_len} should
all be zero/NULL.  Can you help me understand why that is not
possible?

Based on my current understanding, I believe this is something that
should be addressed at the IMA/EVM level and not necessairly at the
LSM layer.

> Adds a `call_int_hook_xattr` macro that fetches and feed the
> `new_xattrs` array with every called hook xattr values.
>
> Adds a `evm_init_hmacs` function which init the EVM hmac using every
> entry of the array contrary to `evm_init_hmac`.
>
> Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
>
> The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
>
> Changes the default return value of the `inode_init_security` hook
> definition to `-EOPNOTSUPP`.
>
> Changes the hook documentation to match the behavior of the LSMs using
> it (only xattr->value is initialised with kmalloc and thus is the only
> one that should be kfreed by the caller).
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
>  include/linux/lsm_hook_defs.h       |  2 +-
>  include/linux/lsm_hooks.h           |  4 ++--
>  security/integrity/evm/evm.h        |  2 ++
>  security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
>  security/integrity/evm/evm_main.c   | 11 ++++++-----
>  security/security.c                 | 29 ++++++++++++++++++++++++++---
>  6 files changed, 59 insertions(+), 12 deletions(-)

-- 
paul-moore.com

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Mickaël Salaün]

On 20/10/2022 15:55, Nicolas Bouchinet wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Fixes a NULL pointer dereference occuring in the

As pointed out by checkpatch.pl, there is a typo.


> `evm_protected_xattr_common` function of the EVM LSM. The bug is
> triggered if a `inode_init_security` hook returns 0 without initializing
> the given `struct xattr` fields (which is the case of BPF) and if no
> other LSM overrides thoses fields after. This also leads to memory
> leaks.

Could you give more details how to trigger this bug and to test this fix?


> 
> Adds a `call_int_hook_xattr` macro that fetches and feed the
> `new_xattrs` array with every called hook xattr values.
> 
> Adds a `evm_init_hmacs` function which init the EVM hmac using every
> entry of the array contrary to `evm_init_hmac`.
> 
> Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> 
> The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> 
> Changes the default return value of the `inode_init_security` hook
> definition to `-EOPNOTSUPP`.
> 
> Changes the hook documentation to match the behavior of the LSMs using
> it (only xattr->value is initialised with kmalloc and thus is the only
> one that should be kfreed by the caller).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
>   include/linux/lsm_hook_defs.h       |  2 +-
>   include/linux/lsm_hooks.h           |  4 ++--
>   security/integrity/evm/evm.h        |  2 ++
>   security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
>   security/integrity/evm/evm_main.c   | 11 ++++++-----
>   security/security.c                 | 29 ++++++++++++++++++++++++++---
>   6 files changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 806448173033..e5dd0c0f6345 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
>   	 unsigned int obj_type)
>   LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
>   LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
> -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
> +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
>   	 struct inode *dir, const struct qstr *qstr, const char **name,
>   	 void **value, size_t *len)
>   LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 84a0d7e02176..95aff9383de1 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -229,8 +229,8 @@
>    *	This hook is called by the fs code as part of the inode creation
>    *	transaction and provides for atomic labeling of the inode, unlike
>    *	the post_create/mkdir/... hooks called by the VFS.  The hook function
> - *	is expected to allocate the name and value via kmalloc, with the caller
> - *	being responsible for calling kfree after using them.
> + *	is expected to allocate the value via kmalloc, with the caller
> + *	being responsible for calling kfree after using it.
>    *	If the security module does not use security attributes or does
>    *	not wish to put a security attribute on this particular inode,
>    *	then it should return -EOPNOTSUPP to skip this processing.
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index f8b8c5004fc7..a2f9886e924d 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>   		  struct evm_digest *data);
>   int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
>   		  char *hmac_val);
> +int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
> +		  char *hmac_val);
>   int evm_init_secfs(void);
>   
>   #endif
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index 708de9656bbd..e5a34306cab6 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
>   	return rc;
>   }
>   
> -

This kind of cosmetic change should not be part of this patch.


>   /*
>    * Calculate the hmac and update security.evm xattr
>    *
> @@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>   	return rc;
>   }
>   
> +int evm_protected_xattr(const char *req_xattr_name);
> +
> +int evm_init_hmacs(struct inode *inode, const struct xattr *lsm_xattrs,
> +		  char *hmac_val)
> +{
> +	struct shash_desc *desc;
> +
> +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> +	if (IS_ERR(desc)) {
> +		pr_info("init_desc failed\n");
> +		return PTR_ERR(desc);
> +	}
> +
> +	for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
> +		if (evm_protected_xattr(lsm_xattrs[i].name))
> +			crypto_shash_update(desc, lsm_xattrs[i].value, lsm_xattrs[i].value_len);
> +	}
> +	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> +	kfree(desc);
> +	return 0;
> +}
> +
>   int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
>   		  char *hmac_val)
>   {
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 2e6fb6e2ffd2..bb071c55d656 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
>   	int found = 0;
>   	struct xattr_list *xattr;
>   
> +	if (!req_xattr_name)
> +		return found;
>   	namelen = strlen(req_xattr_name);
>   	list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) {
>   		if (!all_xattrs && !xattr->enabled)
> @@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
>   	return found;
>   }
>   
> -static int evm_protected_xattr(const char *req_xattr_name)
> +int evm_protected_xattr(const char *req_xattr_name)
>   {
>   	return evm_protected_xattr_common(req_xattr_name, false);
>   }
> @@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>    * evm_inode_init_security - initializes security.evm HMAC value
>    */
>   int evm_inode_init_security(struct inode *inode,
> -				 const struct xattr *lsm_xattr,
> +				 const struct xattr *lsm_xattrs,
>   				 struct xattr *evm_xattr)
>   {
>   	struct evm_xattr *xattr_data;
>   	int rc;
>   
> -	if (!(evm_initialized & EVM_INIT_HMAC) ||
> -	    !evm_protected_xattr(lsm_xattr->name))
> +	if (!(evm_initialized & EVM_INIT_HMAC))
>   		return 0;
>   
>   	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> @@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
>   		return -ENOMEM;
>   
>   	xattr_data->data.type = EVM_XATTR_HMAC;
> -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> +	rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
>   	if (rc < 0)
>   		goto out;
>   
> diff --git a/security/security.c b/security/security.c
> index 14d30fec8a00..47012c118536 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,7 +30,7 @@
>   #include <linux/msg.h>
>   #include <net/flow.h>
>   
> -#define MAX_LSM_EVM_XATTR	2
> +#define MAX_LSM_EVM_XATTR	5

I looks like AppArmor is not using the inode_init_security hook. There 
should be a comment explaining why this number is correct, and a runtime 
check (given that a static_assert call is not possible in this case) to 
make sure this is correct at boot time, maybe in security_init().


>   
>   /* How many LSMs were built into the kernel? */
>   #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
> @@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct super_block *sb)
>   	RC;							\
>   })
>   
> +#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({		\
> +	int RC = IRC;						\
> +	int i = 0;						\
> +	do {							\
> +		struct security_hook_list *P;			\
> +								\
> +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> +			RC = P->hook.FUNC(__VA_ARGS__);		\
> +			if (RC == -EOPNOTSUPP)			\
> +				continue;			\
> +			if (RC != 0 && RC != IRC)		\
> +				break;				\
> +			if (i >= MAX_LSM_EVM_XATTR) {		\

You can use `if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))` instead.


> +				RC = -ENOMEM;			\
> +				break;				\
> +			}					\
> +			XATTRS++;				\
> +			i++;					\
> +		}						\
> +	} while (0);						\
> +	RC;							\
> +})

The content of this macro can be included in its only caller: 
security_inode_init_security().


> +
>   /* Security operations */
>   
>   int security_binder_set_context_mgr(const struct cred *mgr)
> @@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>   				     dir, qstr, NULL, NULL, NULL);
>   	memset(new_xattrs, 0, sizeof(new_xattrs));
>   	lsm_xattr = new_xattrs;
> -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> +	ret = call_int_hook_xattr(lsm_xattr, inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
>   						&lsm_xattr->name,
>   						&lsm_xattr->value,
>   						&lsm_xattr->value_len);
> @@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>   		goto out;
>   
>   	evm_xattr = lsm_xattr + 1;
> -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> +	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
>   	if (ret)
>   		goto out;
>   	ret = initxattrs(inode, new_xattrs, fs_data);

This looks good overall.

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Casey Schaufler]

On 10/20/2022 6:55 AM, Nicolas Bouchinet wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>
> Fixes a NULL pointer dereference occuring in the
> `evm_protected_xattr_common` function of the EVM LSM. The bug is
> triggered if a `inode_init_security` hook returns 0 without initializing
> the given `struct xattr` fields (which is the case of BPF) and if no
> other LSM overrides thoses fields after. This also leads to memory
> leaks.
>
> Adds a `call_int_hook_xattr` macro that fetches and feed the
> `new_xattrs` array with every called hook xattr values.
>
> Adds a `evm_init_hmacs` function which init the EVM hmac using every
> entry of the array contrary to `evm_init_hmac`.
>
> Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
>
> The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
>
> Changes the default return value of the `inode_init_security` hook
> definition to `-EOPNOTSUPP`.
>
> Changes the hook documentation to match the behavior of the LSMs using
> it (only xattr->value is initialised with kmalloc and thus is the only
> one that should be kfreed by the caller).
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
>  include/linux/lsm_hook_defs.h       |  2 +-
>  include/linux/lsm_hooks.h           |  4 ++--
>  security/integrity/evm/evm.h        |  2 ++
>  security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
>  security/integrity/evm/evm_main.c   | 11 ++++++-----
>  security/security.c                 | 29 ++++++++++++++++++++++++++---
>  6 files changed, 59 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 806448173033..e5dd0c0f6345 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
>  	 unsigned int obj_type)
>  LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
>  LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
> -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
> +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
>  	 struct inode *dir, const struct qstr *qstr, const char **name,
>  	 void **value, size_t *len)
>  LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 84a0d7e02176..95aff9383de1 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -229,8 +229,8 @@
>   *	This hook is called by the fs code as part of the inode creation
>   *	transaction and provides for atomic labeling of the inode, unlike
>   *	the post_create/mkdir/... hooks called by the VFS.  The hook function
> - *	is expected to allocate the name and value via kmalloc, with the caller
> - *	being responsible for calling kfree after using them.
> + *	is expected to allocate the value via kmalloc, with the caller
> + *	being responsible for calling kfree after using it.
>   *	If the security module does not use security attributes or does
>   *	not wish to put a security attribute on this particular inode,
>   *	then it should return -EOPNOTSUPP to skip this processing.
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index f8b8c5004fc7..a2f9886e924d 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  		  struct evm_digest *data);
>  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
>  		  char *hmac_val);
> +int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
> +		  char *hmac_val);
>  int evm_init_secfs(void);
>  
>  #endif
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index 708de9656bbd..e5a34306cab6 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
>  	return rc;
>  }
>  
> -
>  /*
>   * Calculate the hmac and update security.evm xattr
>   *
> @@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  	return rc;
>  }
>  
> +int evm_protected_xattr(const char *req_xattr_name);
> +
> +int evm_init_hmacs(struct inode *inode, const struct xattr *lsm_xattrs,
> +		  char *hmac_val)
> +{
> +	struct shash_desc *desc;
> +
> +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> +	if (IS_ERR(desc)) {
> +		pr_info("init_desc failed\n");
> +		return PTR_ERR(desc);
> +	}
> +
> +	for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
> +		if (evm_protected_xattr(lsm_xattrs[i].name))
> +			crypto_shash_update(desc, lsm_xattrs[i].value, lsm_xattrs[i].value_len);
> +	}
> +	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> +	kfree(desc);
> +	return 0;
> +}
> +
>  int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
>  		  char *hmac_val)
>  {
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 2e6fb6e2ffd2..bb071c55d656 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
>  	int found = 0;
>  	struct xattr_list *xattr;
>  
> +	if (!req_xattr_name)
> +		return found;
>  	namelen = strlen(req_xattr_name);
>  	list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) {
>  		if (!all_xattrs && !xattr->enabled)
> @@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
>  	return found;
>  }
>  
> -static int evm_protected_xattr(const char *req_xattr_name)
> +int evm_protected_xattr(const char *req_xattr_name)
>  {
>  	return evm_protected_xattr_common(req_xattr_name, false);
>  }
> @@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>   * evm_inode_init_security - initializes security.evm HMAC value
>   */
>  int evm_inode_init_security(struct inode *inode,
> -				 const struct xattr *lsm_xattr,
> +				 const struct xattr *lsm_xattrs,
>  				 struct xattr *evm_xattr)
>  {
>  	struct evm_xattr *xattr_data;
>  	int rc;
>  
> -	if (!(evm_initialized & EVM_INIT_HMAC) ||
> -	    !evm_protected_xattr(lsm_xattr->name))
> +	if (!(evm_initialized & EVM_INIT_HMAC))
>  		return 0;
>  
>  	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> @@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
>  		return -ENOMEM;
>  
>  	xattr_data->data.type = EVM_XATTR_HMAC;
> -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> +	rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
>  	if (rc < 0)
>  		goto out;
>  
> diff --git a/security/security.c b/security/security.c
> index 14d30fec8a00..47012c118536 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,7 +30,7 @@
>  #include <linux/msg.h>
>  #include <net/flow.h>
>  
> -#define MAX_LSM_EVM_XATTR	2
> +#define MAX_LSM_EVM_XATTR	5

#define MAX_LSM_EVM_XATTR \
	2 + /* IMA and EVM */ \
	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
	(IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))

>  
>  /* How many LSMs were built into the kernel? */
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
> @@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct super_block *sb)
>  	RC;							\
>  })
>  
> +#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({		\
> +	int RC = IRC;						\
> +	int i = 0;						\
> +	do {							\
> +		struct security_hook_list *P;			\
> +								\
> +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> +			RC = P->hook.FUNC(__VA_ARGS__);		\
> +			if (RC == -EOPNOTSUPP)			\
> +				continue;			\
> +			if (RC != 0 && RC != IRC)		\
> +				break;				\
> +			if (i >= MAX_LSM_EVM_XATTR) {		\
> +				RC = -ENOMEM;			\
> +				break;				\
> +			}					\
> +			XATTRS++;				\
> +			i++;					\
> +		}						\
> +	} while (0);						\
> +	RC;							\
> +})
> +

No. Please open code this in the one place it is used.

>  /* Security operations */
>  
>  int security_binder_set_context_mgr(const struct cred *mgr)
> @@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  				     dir, qstr, NULL, NULL, NULL);
>  	memset(new_xattrs, 0, sizeof(new_xattrs));
>  	lsm_xattr = new_xattrs;
> -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> +	ret = call_int_hook_xattr(lsm_xattr, inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
>  						&lsm_xattr->name,
>  						&lsm_xattr->value,
>  						&lsm_xattr->value_len);
> @@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  		goto out;
>  
>  	evm_xattr = lsm_xattr + 1;
> -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> +	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
>  	if (ret)
>  		goto out;
>  	ret = initxattrs(inode, new_xattrs, fs_data);

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Mimi Zohar]

On Thu, 2022-10-20 at 15:55 +0200, Nicolas Bouchinet wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Fixes a NULL pointer dereference occuring in the
> `evm_protected_xattr_common` function of the EVM LSM. The bug is
> triggered if a `inode_init_security` hook returns 0 without initializing
> the given `struct xattr` fields (which is the case of BPF) and if no
> other LSM overrides thoses fields after. This also leads to memory
> leaks.
> 
> Adds a `call_int_hook_xattr` macro that fetches and feed the
> `new_xattrs` array with every called hook xattr values.
> 
> Adds a `evm_init_hmacs` function which init the EVM hmac using every
> entry of the array contrary to `evm_init_hmac`.
  
Only EVM portable digital signatures include all of the protected
xattrs.   Refer to commit 8c7a703ec978 ("evm: Verify portable
signatures against all protected xattrs").

> 
> Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.

Won't this break existing EVM hmac usage?

-- 
thanks,

Mimi

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi, thank for your reply,

On Thu, Oct 20, 2022 at 11:02:07AM -0400, Paul Moore wrote:
> On Thu, Oct 20, 2022 at 9:55 AM Nicolas Bouchinet
> <nicolas.bouchinet@clip-os.org> wrote:
> >
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> >
> > Fixes a NULL pointer dereference occuring in the
> > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > triggered if a `inode_init_security` hook returns 0 without initializing
> > the given `struct xattr` fields (which is the case of BPF) and if no
> > other LSM overrides thoses fields after. This also leads to memory
> > leaks.
> 
> You'll have to forgive me, my connection is poor at the moment and my
> time is limited, but why not simply add some additional checking at
> the top of evm_inode_init_security()? The LSM hook already memset()'s
> the passed lsm_attrs to zero so xattr::{name,value,value_len} should
> all be zero/NULL.  Can you help me understand why that is not
> possible?
> 
> Based on my current understanding, I believe this is something that
> should be addressed at the IMA/EVM level and not necessairly at the
> LSM layer.

The NULL pointer dereference occurs in the `evm_protected_xattr_common()`
function which was originaly called in `evm_inode_init_security()`. I
directly fixed this part at the `evm_inode_init_security()` level.

This patch also addresses other problems which partially occurs at the
`security_inode_init_security()` hook level.
More precisely, based on my understanding, the hook is supposed to initialize
every hooked LSM security xattr and next, if evm is enabled, protect them using 
a HMAC algorithm. However, in the current behavior the use of the
`call_int_hook()` macro by `security_inode_init_security()` overwrites the 
previously initialized xattr for each iteration of the `hlist_for_each_entry()`
loop. Thus, only the last security attribute is taken into account by
evm and freed. Checking the NULL pointer at evm level does not solve this
memory leak.

Based on other replies, I inlined the `call_int_hook()` macro directly into the
`security_inode_init_security()` hook.
> 
> > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > `new_xattrs` array with every called hook xattr values.
> >
> > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > entry of the array contrary to `evm_init_hmac`.
> >
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> >
> > The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> > SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> >
> > Changes the default return value of the `inode_init_security` hook
> > definition to `-EOPNOTSUPP`.
> >
> > Changes the hook documentation to match the behavior of the LSMs using
> > it (only xattr->value is initialised with kmalloc and thus is the only
> > one that should be kfreed by the caller).
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> >  include/linux/lsm_hook_defs.h       |  2 +-
> >  include/linux/lsm_hooks.h           |  4 ++--
> >  security/integrity/evm/evm.h        |  2 ++
> >  security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
> >  security/integrity/evm/evm_main.c   | 11 ++++++-----
> >  security/security.c                 | 29 ++++++++++++++++++++++++++---
> >  6 files changed, 59 insertions(+), 12 deletions(-)
> 
> -- 
> paul-moore.com

Thank for your time,

Best regards,

Nicolas Bouchinet

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi Casey,

Thanks for your time and suggestions.

On Thu, Oct 20, 2022 at 09:41:02AM -0700, Casey Schaufler wrote:
> On 10/20/2022 6:55 AM, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> >
> > Fixes a NULL pointer dereference occuring in the
> > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > triggered if a `inode_init_security` hook returns 0 without initializing
> > the given `struct xattr` fields (which is the case of BPF) and if no
> > other LSM overrides thoses fields after. This also leads to memory
> > leaks.
> >
> > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > `new_xattrs` array with every called hook xattr values.
> >
> > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > entry of the array contrary to `evm_init_hmac`.
> >
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> >
> > The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> > SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> >
> > Changes the default return value of the `inode_init_security` hook
> > definition to `-EOPNOTSUPP`.
> >
> > Changes the hook documentation to match the behavior of the LSMs using
> > it (only xattr->value is initialised with kmalloc and thus is the only
> > one that should be kfreed by the caller).
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> >  include/linux/lsm_hook_defs.h       |  2 +-
> >  include/linux/lsm_hooks.h           |  4 ++--
> >  security/integrity/evm/evm.h        |  2 ++
> >  security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
> >  security/integrity/evm/evm_main.c   | 11 ++++++-----
> >  security/security.c                 | 29 ++++++++++++++++++++++++++---
> >  6 files changed, 59 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 806448173033..e5dd0c0f6345 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
> >  	 unsigned int obj_type)
> >  LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
> >  LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
> > -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
> > +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
> >  	 struct inode *dir, const struct qstr *qstr, const char **name,
> >  	 void **value, size_t *len)
> >  LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 84a0d7e02176..95aff9383de1 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -229,8 +229,8 @@
> >   *	This hook is called by the fs code as part of the inode creation
> >   *	transaction and provides for atomic labeling of the inode, unlike
> >   *	the post_create/mkdir/... hooks called by the VFS.  The hook function
> > - *	is expected to allocate the name and value via kmalloc, with the caller
> > - *	being responsible for calling kfree after using them.
> > + *	is expected to allocate the value via kmalloc, with the caller
> > + *	being responsible for calling kfree after using it.
> >   *	If the security module does not use security attributes or does
> >   *	not wish to put a security attribute on this particular inode,
> >   *	then it should return -EOPNOTSUPP to skip this processing.
> > diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> > index f8b8c5004fc7..a2f9886e924d 100644
> > --- a/security/integrity/evm/evm.h
> > +++ b/security/integrity/evm/evm.h
> > @@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> >  		  struct evm_digest *data);
> >  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
> >  		  char *hmac_val);
> > +int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
> > +		  char *hmac_val);
> >  int evm_init_secfs(void);
> >  
> >  #endif
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index 708de9656bbd..e5a34306cab6 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
> >  	return rc;
> >  }
> >  
> > -
> >  /*
> >   * Calculate the hmac and update security.evm xattr
> >   *
> > @@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> >  	return rc;
> >  }
> >  
> > +int evm_protected_xattr(const char *req_xattr_name);
> > +
> > +int evm_init_hmacs(struct inode *inode, const struct xattr *lsm_xattrs,
> > +		  char *hmac_val)
> > +{
> > +	struct shash_desc *desc;
> > +
> > +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> > +	if (IS_ERR(desc)) {
> > +		pr_info("init_desc failed\n");
> > +		return PTR_ERR(desc);
> > +	}
> > +
> > +	for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
> > +		if (evm_protected_xattr(lsm_xattrs[i].name))
> > +			crypto_shash_update(desc, lsm_xattrs[i].value, lsm_xattrs[i].value_len);
> > +	}
> > +	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> > +	kfree(desc);
> > +	return 0;
> > +}
> > +
> >  int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
> >  		  char *hmac_val)
> >  {
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 2e6fb6e2ffd2..bb071c55d656 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
> >  	int found = 0;
> >  	struct xattr_list *xattr;
> >  
> > +	if (!req_xattr_name)
> > +		return found;
> >  	namelen = strlen(req_xattr_name);
> >  	list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) {
> >  		if (!all_xattrs && !xattr->enabled)
> > @@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
> >  	return found;
> >  }
> >  
> > -static int evm_protected_xattr(const char *req_xattr_name)
> > +int evm_protected_xattr(const char *req_xattr_name)
> >  {
> >  	return evm_protected_xattr_common(req_xattr_name, false);
> >  }
> > @@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> >   * evm_inode_init_security - initializes security.evm HMAC value
> >   */
> >  int evm_inode_init_security(struct inode *inode,
> > -				 const struct xattr *lsm_xattr,
> > +				 const struct xattr *lsm_xattrs,
> >  				 struct xattr *evm_xattr)
> >  {
> >  	struct evm_xattr *xattr_data;
> >  	int rc;
> >  
> > -	if (!(evm_initialized & EVM_INIT_HMAC) ||
> > -	    !evm_protected_xattr(lsm_xattr->name))
> > +	if (!(evm_initialized & EVM_INIT_HMAC))
> >  		return 0;
> >  
> >  	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> > @@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
> >  		return -ENOMEM;
> >  
> >  	xattr_data->data.type = EVM_XATTR_HMAC;
> > -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> > +	rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
> >  	if (rc < 0)
> >  		goto out;
> >  
> > diff --git a/security/security.c b/security/security.c
> > index 14d30fec8a00..47012c118536 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -30,7 +30,7 @@
> >  #include <linux/msg.h>
> >  #include <net/flow.h>
> >  
> > -#define MAX_LSM_EVM_XATTR	2
> > +#define MAX_LSM_EVM_XATTR	5
> 
> #define MAX_LSM_EVM_XATTR \
> 	2 + /* IMA and EVM */ \
> 	(IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> 	(IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) + \
> 	(IS_ENABLED(CONFIG_SECURITY_APPARMOR) ? 1 : 0) + \
> 	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
> 

This is neat, I will change my code to your proposition and send the patch later.
> >  
> >  /* How many LSMs were built into the kernel? */
> >  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
> > @@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct super_block *sb)
> >  	RC;							\
> >  })
> >  
> > +#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({		\
> > +	int RC = IRC;						\
> > +	int i = 0;						\
> > +	do {							\
> > +		struct security_hook_list *P;			\
> > +								\
> > +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > +			RC = P->hook.FUNC(__VA_ARGS__);		\
> > +			if (RC == -EOPNOTSUPP)			\
> > +				continue;			\
> > +			if (RC != 0 && RC != IRC)		\
> > +				break;				\
> > +			if (i >= MAX_LSM_EVM_XATTR) {		\
> > +				RC = -ENOMEM;			\
> > +				break;				\
> > +			}					\
> > +			XATTRS++;				\
> > +			i++;					\
> > +		}						\
> > +	} while (0);						\
> > +	RC;							\
> > +})
> > +
> 
> No. Please open code this in the one place it is used.
> 
Ok, done.
> >  /* Security operations */
> >  
> >  int security_binder_set_context_mgr(const struct cred *mgr)
> > @@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> >  				     dir, qstr, NULL, NULL, NULL);
> >  	memset(new_xattrs, 0, sizeof(new_xattrs));
> >  	lsm_xattr = new_xattrs;
> > -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> > +	ret = call_int_hook_xattr(lsm_xattr, inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> >  						&lsm_xattr->name,
> >  						&lsm_xattr->value,
> >  						&lsm_xattr->value_len);
> > @@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> >  		goto out;
> >  
> >  	evm_xattr = lsm_xattr + 1;
> > -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> > +	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
> >  	if (ret)
> >  		goto out;
> >  	ret = initxattrs(inode, new_xattrs, fs_data);

Best regards,

Nicolas Bouchinet

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi Mimi,

Thanks for the IMA/EVM project which I enjoy very much.

On Thu, Oct 20, 2022 at 03:51:38PM -0400, Mimi Zohar wrote:
> On Thu, 2022-10-20 at 15:55 +0200, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > Fixes a NULL pointer dereference occuring in the
> > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > triggered if a `inode_init_security` hook returns 0 without initializing
> > the given `struct xattr` fields (which is the case of BPF) and if no
> > other LSM overrides thoses fields after. This also leads to memory
> > leaks.
> > 
> > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > `new_xattrs` array with every called hook xattr values.
> > 
> > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > entry of the array contrary to `evm_init_hmac`.
>   
> Only EVM portable digital signatures include all of the protected
> xattrs.   Refer to commit 8c7a703ec978 ("evm: Verify portable
> signatures against all protected xattrs").
> 
Sorry, maybe I was not clear enough, the proposed patch does not change the
set of the protected security xattrs initialized by the LSMs and processed by EVM.

As I explained to Paul, based on my understanding, the `security_inode_init_security()`
hook is supposed to initialize every hooked LSM security xattr and next,
if evm is enabled, protect them using a HMAC algorithm.
However, in the current implementation, the use of the `call_int_hook()` macro by
`security_inode_init_security()` overwrites the previously initialized xattr for
each iteration of the `hlist_for_each_entry()` loop.

I have noticed that more than one LSM may initialize a security xattr at a time,
eg. SELinux + BPF.

IMHO my supplementary `evm_init_hmacs()` function name is a bit confusing, I would
enjoy if you have a better proposition. Note that `evm_init_hmacs()` have the same
behavior as `evm_init_hmac()` if only one security xattr is given as a parameter.

> > 
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> 
> Won't this break existing EVM hmac usage?
I might be wrong, but as far as I understand it, the only working condition for
EVM now is when only one security xattr is involved, otherwise there will have
a mismatch between the initialization and the verification.
Indeed, the verification takes into account every security xattr written in its
refering dentry.

> 
> -- 
> thanks,
> 
> Mimi
> 

Thanks for your time,
Best regards,

Nicolas Bouchinet

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Roberto Sassu]

On Thu, 2022-10-20 at 15:55 +0200, Nicolas Bouchinet wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Fixes a NULL pointer dereference occuring in the
> `evm_protected_xattr_common` function of the EVM LSM. The bug is
> triggered if a `inode_init_security` hook returns 0 without
> initializing
> the given `struct xattr` fields (which is the case of BPF) and if no
> other LSM overrides thoses fields after. This also leads to memory
> leaks.

+ eBPF mailing list, KP

Looking at include/linux/lsm_hooks.h:

 * @inode_init_security:

[...]

 *	If the security module does not use security attributes or does
 *	not wish to put a security attribute on this particular inode,
 *	then it should return -EOPNOTSUPP to skip this processing.

[...]

 *	Returns 0 if @name and @value have been successfully set,
 *	-EOPNOTSUPP if no security attribute is needed, or

In my opinion, it should be responsibility of the eBPF infrastructure
to ensure that this is true (meaning that it cannot let security
modules attach to that hook without an additional check).

What do you think?

Nicolas, in the past I addressed the same issue of lacking support of
multiple LSMs providing an xattr at inode creation time.

Would this patch set be fine for you, or you would still do
differently?

https://lore.kernel.org/all/20210427113732.471066-1-roberto.sassu@huawei.com/

At the end of the cover letter, you can find also a TestLSM I developed
to ensure that support for multiple LSMs works correctly. I also tested
the patch set with the SELinux and SMACK test suites.

Thanks

Roberto

> Adds a `call_int_hook_xattr` macro that fetches and feed the
> `new_xattrs` array with every called hook xattr values.
> 
> Adds a `evm_init_hmacs` function which init the EVM hmac using every
> entry of the array contrary to `evm_init_hmac`.
> 
> Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> 
> The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room
> for
> SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> 
> Changes the default return value of the `inode_init_security` hook
> definition to `-EOPNOTSUPP`.
> 
> Changes the hook documentation to match the behavior of the LSMs
> using
> it (only xattr->value is initialised with kmalloc and thus is the
> only
> one that should be kfreed by the caller).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
>  include/linux/lsm_hook_defs.h       |  2 +-
>  include/linux/lsm_hooks.h           |  4 ++--
>  security/integrity/evm/evm.h        |  2 ++
>  security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
>  security/integrity/evm/evm_main.c   | 11 ++++++-----
>  security/security.c                 | 29 ++++++++++++++++++++++++++-
> --
>  6 files changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/lsm_hook_defs.h
> b/include/linux/lsm_hook_defs.h
> index 806448173033..e5dd0c0f6345 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path
> *path, u64 mask,
>  	 unsigned int obj_type)
>  LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
>  LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode
> *inode)
> -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
> +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
>  	 struct inode *dir, const struct qstr *qstr, const char **name,
>  	 void **value, size_t *len)
>  LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 84a0d7e02176..95aff9383de1 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -229,8 +229,8 @@
>   *	This hook is called by the fs code as part of the inode
> creation
>   *	transaction and provides for atomic labeling of the inode,
> unlike
>   *	the post_create/mkdir/... hooks called by the VFS.  The hook
> function
> - *	is expected to allocate the name and value via kmalloc, with
> the caller
> - *	being responsible for calling kfree after using them.
> + *	is expected to allocate the value via kmalloc, with the caller
> + *	being responsible for calling kfree after using it.
>   *	If the security module does not use security attributes or does
>   *	not wish to put a security attribute on this particular inode,
>   *	then it should return -EOPNOTSUPP to skip this processing.
> diff --git a/security/integrity/evm/evm.h
> b/security/integrity/evm/evm.h
> index f8b8c5004fc7..a2f9886e924d 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char
> *req_xattr_name,
>  		  struct evm_digest *data);
>  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
>  		  char *hmac_val);
> +int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
> +		  char *hmac_val);
>  int evm_init_secfs(void);
>  
>  #endif
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 708de9656bbd..e5a34306cab6 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry
> *dentry, struct inode *inode)
>  	return rc;
>  }
>  
> -
>  /*
>   * Calculate the hmac and update security.evm xattr
>   *
> @@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry,
> const char *xattr_name,
>  	return rc;
>  }
>  
> +int evm_protected_xattr(const char *req_xattr_name);
> +
> +int evm_init_hmacs(struct inode *inode, const struct xattr
> *lsm_xattrs,
> +		  char *hmac_val)
> +{
> +	struct shash_desc *desc;
> +
> +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> +	if (IS_ERR(desc)) {
> +		pr_info("init_desc failed\n");
> +		return PTR_ERR(desc);
> +	}
> +
> +	for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
> +		if (evm_protected_xattr(lsm_xattrs[i].name))
> +			crypto_shash_update(desc, lsm_xattrs[i].value,
> lsm_xattrs[i].value_len);
> +	}
> +	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> +	kfree(desc);
> +	return 0;
> +}
> +
>  int evm_init_hmac(struct inode *inode, const struct xattr
> *lsm_xattr,
>  		  char *hmac_val)
>  {
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> index 2e6fb6e2ffd2..bb071c55d656 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char
> *req_xattr_name,
>  	int found = 0;
>  	struct xattr_list *xattr;
>  
> +	if (!req_xattr_name)
> +		return found;
>  	namelen = strlen(req_xattr_name);
>  	list_for_each_entry_lockless(xattr, &evm_config_xattrnames,
> list) {
>  		if (!all_xattrs && !xattr->enabled)
> @@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char
> *req_xattr_name,
>  	return found;
>  }
>  
> -static int evm_protected_xattr(const char *req_xattr_name)
> +int evm_protected_xattr(const char *req_xattr_name)
>  {
>  	return evm_protected_xattr_common(req_xattr_name, false);
>  }
> @@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
>   * evm_inode_init_security - initializes security.evm HMAC value
>   */
>  int evm_inode_init_security(struct inode *inode,
> -				 const struct xattr *lsm_xattr,
> +				 const struct xattr *lsm_xattrs,
>  				 struct xattr *evm_xattr)
>  {
>  	struct evm_xattr *xattr_data;
>  	int rc;
>  
> -	if (!(evm_initialized & EVM_INIT_HMAC) ||
> -	    !evm_protected_xattr(lsm_xattr->name))
> +	if (!(evm_initialized & EVM_INIT_HMAC))
>  		return 0;
>  
>  	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> @@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
>  		return -ENOMEM;
>  
>  	xattr_data->data.type = EVM_XATTR_HMAC;
> -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> +	rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
>  	if (rc < 0)
>  		goto out;
>  
> diff --git a/security/security.c b/security/security.c
> index 14d30fec8a00..47012c118536 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,7 +30,7 @@
>  #include <linux/msg.h>
>  #include <net/flow.h>
>  
> -#define MAX_LSM_EVM_XATTR	2
> +#define MAX_LSM_EVM_XATTR	5
>  
>  /* How many LSMs were built into the kernel? */
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
> @@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct
> super_block *sb)
>  	RC;							\
>  })
>  
> +#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({		
> \
> +	int RC = IRC;						\
> +	int i = 0;						\
> +	do {							\
> +		struct security_hook_list *P;			\
> +								\
> +		hlist_for_each_entry(P, &security_hook_heads.FUNC,
> list) { \
> +			RC = P->hook.FUNC(__VA_ARGS__);		\
> +			if (RC == -EOPNOTSUPP)			\
> +				continue;			\
> +			if (RC != 0 && RC != IRC)		\
> +				break;				\
> +			if (i >= MAX_LSM_EVM_XATTR) {		\
> +				RC = -ENOMEM;			\
> +				break;				\
> +			}					\
> +			XATTRS++;				\
> +			i++;					\
> +		}						\
> +	} while (0);						\
> +	RC;							\
> +})
> +
>  /* Security operations */
>  
>  int security_binder_set_context_mgr(const struct cred *mgr)
> @@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode
> *inode, struct inode *dir,
>  				     dir, qstr, NULL, NULL, NULL);
>  	memset(new_xattrs, 0, sizeof(new_xattrs));
>  	lsm_xattr = new_xattrs;
> -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
> dir, qstr,
> +	ret = call_int_hook_xattr(lsm_xattr, inode_init_security,
> -EOPNOTSUPP, inode, dir, qstr,
>  						&lsm_xattr->name,
>  						&lsm_xattr->value,
>  						&lsm_xattr->value_len);
> @@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode
> *inode, struct inode *dir,
>  		goto out;
>  
>  	evm_xattr = lsm_xattr + 1;
> -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> +	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
>  	if (ret)
>  		goto out;
>  	ret = initxattrs(inode, new_xattrs, fs_data);

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi Mickaël, thanks for you review,

On Thu, Oct 20, 2022 at 05:14:16PM +0200, Mickaël Salaün wrote:
> 
> On 20/10/2022 15:55, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > Fixes a NULL pointer dereference occuring in the
> 
> As pointed out by checkpatch.pl, there is a typo.
Taken into account.
> 
> 
> > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > triggered if a `inode_init_security` hook returns 0 without initializing
> > the given `struct xattr` fields (which is the case of BPF) and if no
> > other LSM overrides thoses fields after. This also leads to memory
> > leaks.
> 
> Could you give more details how to trigger this bug and to test this fix?
To trigger the bug, I configured a fedora VM using mkinitcpio because fedora
has every tool to configure EVM. I then injected the archlinux dracut module tree
(/usr/lib/dracut/modules.d) in it.

The only thing to do next is to configure IMA/EVM enough for it to be enabled at boot.
You may follow this https://en.opensuse.org/SDB:Ima_evm#EVM_Protection_Using_HMACs
tutorial up to the end of the "Creation of Kernel Master Key and EVM HMAC key" part.

Next, regenerate your initrd using dracut and its integrity module
`dracut --add integrity /PATH/TO/initrd --force --verbose` and reboot.

Here is the stacktrace you should get:

```
[  OK  ] Finished dracut-pre-pivot.…dracut pre-pivot and cleanup hook.
[   38.159571] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   38.160155] #PF: supervisor read access in kernel mode
[   38.160567] #PF: error_code(0x0000) - not-present page
[   38.160988] PGD 0 P4D 0
[   38.161209] Oops: 0000 [#1] PREEMPT SMP NOPTI
[   38.161548] CPU: 0 PID: 1 Comm: systemd Not tainted 5.19.9-200.fc36.x86_64 #1
[   38.162112] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[   38.162713] RIP: 0010:strlen+0x0/0x20
[   38.163002] Code: b6 07 38 d0 74 14 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 c3 cc cc cc cc 48 89 f8 c3 cc cc cc cc 0f 1f 84 00 00 00 00 00 <80> 3f 00 74 14 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 cc
[   38.164380] RSP: 0018:ffffc9000001fcf8 EFLAGS: 00010246
[   38.164770] RAX: 0000000000000000 RBX: ffffc9000001fd78 RCX: ffffc9000001fd78
[   38.165280] RDX: ffffc9000001fd90 RSI: 0000000000000000 RDI: 0000000000000000
[   38.165813] RBP: 0000000000000000 R08: ffffc9000001fd80 R09: ffffc9000001fd88
[   38.166308] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8880061c18c8
[   38.166772] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[   38.167238] FS:  00007f59e2addb40(0000) GS:ffff88803d400000(0000) knlGS:0000000000000000
[   38.167871] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   38.168322] CR2: 0000000000000000 CR3: 000000000322e002 CR4: 0000000000370ef0
[   38.168876] Call Trace:
[   38.169088]  <TASK>
[   38.169246]  evm_protected_xattr_common+0x1a/0xb0
[   38.169610]  evm_inode_init_security+0x32/0xb0
[   38.169983]  security_inode_init_security+0xd3/0x130
[   38.170373]  ? shmem_encode_fh+0x90/0x90
[   38.170676]  shmem_symlink+0x7c/0x290
[   38.170948]  vfs_symlink+0x5d/0xe0
[   38.171229]  do_symlinkat+0xf7/0x110
[   38.171487]  __x64_sys_symlink+0x37/0x40
[   38.171779]  do_syscall_64+0x58/0x80
[   38.172071]  ? syscall_exit_to_user_mode+0x17/0x40
[   38.172462]  ? do_syscall_64+0x67/0x80
[   38.172738]  ? syscall_exit_to_user_mode+0x17/0x40
[   38.173116]  ? do_syscall_64+0x67/0x80
[   38.173405]  ? do_syscall_64+0x67/0x80
[   38.173672]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   38.174021] RIP: 0033:0x7f59e35f405b
[   38.174309] Code: f0 ff ff 73 01 c3 48 8b 0d c2 4d 0f 00 f7 d8 64 89 01 48 83 c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa b8 58 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 95 4d 0f 00 f7 d8 64 89 01 48
[   38.175713] RSP: 002b:00007ffc5960eb28 EFLAGS: 00000246 ORIG_RAX: 0000000000000058
[   38.176310] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f59e35f405b
[   38.176879] RDX: 000055cef7249570 RSI: 000055cbaba83260 RDI: 000055cbaba54ac0
[   38.177393] RBP: 000055cbaba5e9d0 R08: 000055cbabac04b0 R09: 0000000000000000
[   38.177968] R10: 00007ffc5960e849 R11: 0000000000000246 R12: 000055cbaba54ac0
[   38.178558] R13: 000055cbaba5e9d0 R14: 000055cbab9fae00 R15: 0000000000000001
[   38.179148]  </TASK>
[   38.179341] Modules linked in: dm_crypt crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel serio_raw virtio_scsi qemu_fw_cfg
[   38.180224] CR2: 0000000000000000
[   38.180501] ---[ end trace 0000000000000000 ]---
[   38.180879] RIP: 0010:strlen+0x0/0x20
[   38.181177] Code: b6 07 38 d0 74 14 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 c3 cc cc cc cc 48 89 f8 c3 cc cc cc cc 0f 1f 84 00 00 00 00 00 <80> 3f 00 74 14 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 cc
[   38.182565] RSP: 0018:ffffc9000001fcf8 EFLAGS: 00010246
[   38.182944] RAX: 0000000000000000 RBX: ffffc9000001fd78 RCX: ffffc9000001fd78
[   38.183491] RDX: ffffc9000001fd90 RSI: 0000000000000000 RDI: 0000000000000000
[   38.184035] RBP: 0000000000000000 R08: ffffc9000001fd80 R09: ffffc9000001fd88
[   38.184556] R10: 0000000000000000 R11: 0000000000000000 R12: ffff8880061c18c8
[   38.185141] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[   38.185724] FS:  00007f59e2addb40(0000) GS:ffff88803d400000(0000) knlGS:0000000000000000
[   38.186311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   38.186775] CR2: 0000000000000000 CR3: 000000000322e002 CR4: 0000000000370ef0
[   38.187804] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
[   38.188443] Kernel Offset: disabled
[   38.188705] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009 ]---
```

> 
> 
> > 
> > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > `new_xattrs` array with every called hook xattr values.
> > 
> > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > entry of the array contrary to `evm_init_hmac`.
> > 
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> > 
> > The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> > SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> > 
> > Changes the default return value of the `inode_init_security` hook
> > definition to `-EOPNOTSUPP`.
> > 
> > Changes the hook documentation to match the behavior of the LSMs using
> > it (only xattr->value is initialised with kmalloc and thus is the only
> > one that should be kfreed by the caller).
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> >   include/linux/lsm_hook_defs.h       |  2 +-
> >   include/linux/lsm_hooks.h           |  4 ++--
> >   security/integrity/evm/evm.h        |  2 ++
> >   security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
> >   security/integrity/evm/evm_main.c   | 11 ++++++-----
> >   security/security.c                 | 29 ++++++++++++++++++++++++++---
> >   6 files changed, 59 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 806448173033..e5dd0c0f6345 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
> >   	 unsigned int obj_type)
> >   LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
> >   LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
> > -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
> > +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
> >   	 struct inode *dir, const struct qstr *qstr, const char **name,
> >   	 void **value, size_t *len)
> >   LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 84a0d7e02176..95aff9383de1 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -229,8 +229,8 @@
> >    *	This hook is called by the fs code as part of the inode creation
> >    *	transaction and provides for atomic labeling of the inode, unlike
> >    *	the post_create/mkdir/... hooks called by the VFS.  The hook function
> > - *	is expected to allocate the name and value via kmalloc, with the caller
> > - *	being responsible for calling kfree after using them.
> > + *	is expected to allocate the value via kmalloc, with the caller
> > + *	being responsible for calling kfree after using it.
> >    *	If the security module does not use security attributes or does
> >    *	not wish to put a security attribute on this particular inode,
> >    *	then it should return -EOPNOTSUPP to skip this processing.
> > diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> > index f8b8c5004fc7..a2f9886e924d 100644
> > --- a/security/integrity/evm/evm.h
> > +++ b/security/integrity/evm/evm.h
> > @@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> >   		  struct evm_digest *data);
> >   int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
> >   		  char *hmac_val);
> > +int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
> > +		  char *hmac_val);
> >   int evm_init_secfs(void);
> >   #endif
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index 708de9656bbd..e5a34306cab6 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
> >   	return rc;
> >   }
> > -
> 
> This kind of cosmetic change should not be part of this patch.
Removed, thanks.
> 
> 
> >   /*
> >    * Calculate the hmac and update security.evm xattr
> >    *
> > @@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> >   	return rc;
> >   }
> > +int evm_protected_xattr(const char *req_xattr_name);
> > +
> > +int evm_init_hmacs(struct inode *inode, const struct xattr *lsm_xattrs,
> > +		  char *hmac_val)
> > +{
> > +	struct shash_desc *desc;
> > +
> > +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> > +	if (IS_ERR(desc)) {
> > +		pr_info("init_desc failed\n");
> > +		return PTR_ERR(desc);
> > +	}
> > +
> > +	for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
> > +		if (evm_protected_xattr(lsm_xattrs[i].name))
> > +			crypto_shash_update(desc, lsm_xattrs[i].value, lsm_xattrs[i].value_len);
> > +	}
> > +	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> > +	kfree(desc);
> > +	return 0;
> > +}
> > +
> >   int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
> >   		  char *hmac_val)
> >   {
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 2e6fb6e2ffd2..bb071c55d656 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
> >   	int found = 0;
> >   	struct xattr_list *xattr;
> > +	if (!req_xattr_name)
> > +		return found;
> >   	namelen = strlen(req_xattr_name);
> >   	list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) {
> >   		if (!all_xattrs && !xattr->enabled)
> > @@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char *req_xattr_name,
> >   	return found;
> >   }
> > -static int evm_protected_xattr(const char *req_xattr_name)
> > +int evm_protected_xattr(const char *req_xattr_name)
> >   {
> >   	return evm_protected_xattr_common(req_xattr_name, false);
> >   }
> > @@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> >    * evm_inode_init_security - initializes security.evm HMAC value
> >    */
> >   int evm_inode_init_security(struct inode *inode,
> > -				 const struct xattr *lsm_xattr,
> > +				 const struct xattr *lsm_xattrs,
> >   				 struct xattr *evm_xattr)
> >   {
> >   	struct evm_xattr *xattr_data;
> >   	int rc;
> > -	if (!(evm_initialized & EVM_INIT_HMAC) ||
> > -	    !evm_protected_xattr(lsm_xattr->name))
> > +	if (!(evm_initialized & EVM_INIT_HMAC))
> >   		return 0;
> >   	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> > @@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
> >   		return -ENOMEM;
> >   	xattr_data->data.type = EVM_XATTR_HMAC;
> > -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> > +	rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
> >   	if (rc < 0)
> >   		goto out;
> > diff --git a/security/security.c b/security/security.c
> > index 14d30fec8a00..47012c118536 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -30,7 +30,7 @@
> >   #include <linux/msg.h>
> >   #include <net/flow.h>
> > -#define MAX_LSM_EVM_XATTR	2
> > +#define MAX_LSM_EVM_XATTR	5
> 
> I looks like AppArmor is not using the inode_init_security hook. There
> should be a comment explaining why this number is correct, and a runtime
> check (given that a static_assert call is not possible in this case) to make
> sure this is correct at boot time, maybe in security_init().

Your right, sorry for this, I took this into account and removed AppArmor from the count.
> 
> 
> >   /* How many LSMs were built into the kernel? */
> >   #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
> > @@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct super_block *sb)
> >   	RC;							\
> >   })
> > +#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({		\
> > +	int RC = IRC;						\
> > +	int i = 0;						\
> > +	do {							\
> > +		struct security_hook_list *P;			\
> > +								\
> > +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > +			RC = P->hook.FUNC(__VA_ARGS__);		\
> > +			if (RC == -EOPNOTSUPP)			\
> > +				continue;			\
> > +			if (RC != 0 && RC != IRC)		\
> > +				break;				\
> > +			if (i >= MAX_LSM_EVM_XATTR) {		\
> 
> You can use `if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))` instead.
Neat, done.
> 
> 
> > +				RC = -ENOMEM;			\
> > +				break;				\
> > +			}					\
> > +			XATTRS++;				\
> > +			i++;					\
> > +		}						\
> > +	} while (0);						\
> > +	RC;							\
> > +})
> 
> The content of this macro can be included in its only caller:
> security_inode_init_security().
Casey and Paul also asked for this, taken into account.
> 
> 
> > +
> >   /* Security operations */
> >   int security_binder_set_context_mgr(const struct cred *mgr)
> > @@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> >   				     dir, qstr, NULL, NULL, NULL);
> >   	memset(new_xattrs, 0, sizeof(new_xattrs));
> >   	lsm_xattr = new_xattrs;
> > -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> > +	ret = call_int_hook_xattr(lsm_xattr, inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> >   						&lsm_xattr->name,
> >   						&lsm_xattr->value,
> >   						&lsm_xattr->value_len);
> > @@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> >   		goto out;
> >   	evm_xattr = lsm_xattr + 1;
> > -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> > +	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
> >   	if (ret)
> >   		goto out;
> >   	ret = initxattrs(inode, new_xattrs, fs_data);
> 
> This looks good overall.

Thanks for your time,

Best regards,

Nicolas Bouchinet

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi Roberto,

On Fri, Oct 21, 2022 at 04:02:11PM +0200, Roberto Sassu wrote:
> On Thu, 2022-10-20 at 15:55 +0200, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > Fixes a NULL pointer dereference occuring in the
> > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > triggered if a `inode_init_security` hook returns 0 without
> > initializing
> > the given `struct xattr` fields (which is the case of BPF) and if no
> > other LSM overrides thoses fields after. This also leads to memory
> > leaks.
> 
> + eBPF mailing list, KP
> 
> Looking at include/linux/lsm_hooks.h:
> 
>  * @inode_init_security:
> 
> [...]
> 
>  *	If the security module does not use security attributes or does
>  *	not wish to put a security attribute on this particular inode,
>  *	then it should return -EOPNOTSUPP to skip this processing.
> 
> [...]
> 
>  *	Returns 0 if @name and @value have been successfully set,
>  *	-EOPNOTSUPP if no security attribute is needed, or
> 
> In my opinion, it should be responsibility of the eBPF infrastructure
> to ensure that this is true (meaning that it cannot let security
> modules attach to that hook without an additional check).
> 
> What do you think?
> 
> Nicolas, in the past I addressed the same issue of lacking support of
> multiple LSMs providing an xattr at inode creation time.
> 
> Would this patch set be fine for you, or you would still do
> differently?

IMHO, changing the default return value of the hook to `-EOPNOTSUPP`
in the lsm_hook_defs.h should be enough, the LSM implementation however
does not follows the current documentation as returning `-EOPNOTSUPP`
skips the whole processing of LSMs `xattrs`. This is why I check the
return value of each LSM `inode_init_security()` functions in the patch.
> 
> https://lore.kernel.org/all/20210427113732.471066-1-roberto.sassu@huawei.com/
> 
> At the end of the cover letter, you can find also a TestLSM I developed
> to ensure that support for multiple LSMs works correctly. I also tested
> the patch set with the SELinux and SMACK test suites.

Thanks, I'll give it a look !
> 
> Thanks
> 
> Roberto
> 
> > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > `new_xattrs` array with every called hook xattr values.
> > 
> > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > entry of the array contrary to `evm_init_hmac`.
> > 
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> > 
> > The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room
> > for
> > SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> > 
> > Changes the default return value of the `inode_init_security` hook
> > definition to `-EOPNOTSUPP`.
> > 
> > Changes the hook documentation to match the behavior of the LSMs
> > using
> > it (only xattr->value is initialised with kmalloc and thus is the
> > only
> > one that should be kfreed by the caller).
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> >  include/linux/lsm_hook_defs.h       |  2 +-
> >  include/linux/lsm_hooks.h           |  4 ++--
> >  security/integrity/evm/evm.h        |  2 ++
> >  security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
> >  security/integrity/evm/evm_main.c   | 11 ++++++-----
> >  security/security.c                 | 29 ++++++++++++++++++++++++++-
> > --
> >  6 files changed, 59 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/linux/lsm_hook_defs.h
> > b/include/linux/lsm_hook_defs.h
> > index 806448173033..e5dd0c0f6345 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path
> > *path, u64 mask,
> >  	 unsigned int obj_type)
> >  LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
> >  LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode
> > *inode)
> > -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
> > +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
> >  	 struct inode *dir, const struct qstr *qstr, const char **name,
> >  	 void **value, size_t *len)
> >  LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 84a0d7e02176..95aff9383de1 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -229,8 +229,8 @@
> >   *	This hook is called by the fs code as part of the inode
> > creation
> >   *	transaction and provides for atomic labeling of the inode,
> > unlike
> >   *	the post_create/mkdir/... hooks called by the VFS.  The hook
> > function
> > - *	is expected to allocate the name and value via kmalloc, with
> > the caller
> > - *	being responsible for calling kfree after using them.
> > + *	is expected to allocate the value via kmalloc, with the caller
> > + *	being responsible for calling kfree after using it.
> >   *	If the security module does not use security attributes or does
> >   *	not wish to put a security attribute on this particular inode,
> >   *	then it should return -EOPNOTSUPP to skip this processing.
> > diff --git a/security/integrity/evm/evm.h
> > b/security/integrity/evm/evm.h
> > index f8b8c5004fc7..a2f9886e924d 100644
> > --- a/security/integrity/evm/evm.h
> > +++ b/security/integrity/evm/evm.h
> > @@ -60,6 +60,8 @@ int evm_calc_hash(struct dentry *dentry, const char
> > *req_xattr_name,
> >  		  struct evm_digest *data);
> >  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
> >  		  char *hmac_val);
> > +int evm_init_hmacs(struct inode *inode, const struct xattr *xattrs,
> > +		  char *hmac_val);
> >  int evm_init_secfs(void);
> >  
> >  #endif
> > diff --git a/security/integrity/evm/evm_crypto.c
> > b/security/integrity/evm/evm_crypto.c
> > index 708de9656bbd..e5a34306cab6 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -347,7 +347,6 @@ static int evm_is_immutable(struct dentry
> > *dentry, struct inode *inode)
> >  	return rc;
> >  }
> >  
> > -
> >  /*
> >   * Calculate the hmac and update security.evm xattr
> >   *
> > @@ -385,6 +384,28 @@ int evm_update_evmxattr(struct dentry *dentry,
> > const char *xattr_name,
> >  	return rc;
> >  }
> >  
> > +int evm_protected_xattr(const char *req_xattr_name);
> > +
> > +int evm_init_hmacs(struct inode *inode, const struct xattr
> > *lsm_xattrs,
> > +		  char *hmac_val)
> > +{
> > +	struct shash_desc *desc;
> > +
> > +	desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
> > +	if (IS_ERR(desc)) {
> > +		pr_info("init_desc failed\n");
> > +		return PTR_ERR(desc);
> > +	}
> > +
> > +	for (int i = 0; lsm_xattrs[i].value != NULL; i++) {
> > +		if (evm_protected_xattr(lsm_xattrs[i].name))
> > +			crypto_shash_update(desc, lsm_xattrs[i].value,
> > lsm_xattrs[i].value_len);
> > +	}
> > +	hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
> > +	kfree(desc);
> > +	return 0;
> > +}
> > +
> >  int evm_init_hmac(struct inode *inode, const struct xattr
> > *lsm_xattr,
> >  		  char *hmac_val)
> >  {
> > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > index 2e6fb6e2ffd2..bb071c55d656 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -284,6 +284,8 @@ static int evm_protected_xattr_common(const char
> > *req_xattr_name,
> >  	int found = 0;
> >  	struct xattr_list *xattr;
> >  
> > +	if (!req_xattr_name)
> > +		return found;
> >  	namelen = strlen(req_xattr_name);
> >  	list_for_each_entry_lockless(xattr, &evm_config_xattrnames,
> > list) {
> >  		if (!all_xattrs && !xattr->enabled)
> > @@ -305,7 +307,7 @@ static int evm_protected_xattr_common(const char
> > *req_xattr_name,
> >  	return found;
> >  }
> >  
> > -static int evm_protected_xattr(const char *req_xattr_name)
> > +int evm_protected_xattr(const char *req_xattr_name)
> >  {
> >  	return evm_protected_xattr_common(req_xattr_name, false);
> >  }
> > @@ -835,14 +837,13 @@ void evm_inode_post_setattr(struct dentry
> > *dentry, int ia_valid)
> >   * evm_inode_init_security - initializes security.evm HMAC value
> >   */
> >  int evm_inode_init_security(struct inode *inode,
> > -				 const struct xattr *lsm_xattr,
> > +				 const struct xattr *lsm_xattrs,
> >  				 struct xattr *evm_xattr)
> >  {
> >  	struct evm_xattr *xattr_data;
> >  	int rc;
> >  
> > -	if (!(evm_initialized & EVM_INIT_HMAC) ||
> > -	    !evm_protected_xattr(lsm_xattr->name))
> > +	if (!(evm_initialized & EVM_INIT_HMAC))
> >  		return 0;
> >  
> >  	xattr_data = kzalloc(sizeof(*xattr_data), GFP_NOFS);
> > @@ -850,7 +851,7 @@ int evm_inode_init_security(struct inode *inode,
> >  		return -ENOMEM;
> >  
> >  	xattr_data->data.type = EVM_XATTR_HMAC;
> > -	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
> > +	rc = evm_init_hmacs(inode, lsm_xattrs, xattr_data->digest);
> >  	if (rc < 0)
> >  		goto out;
> >  
> > diff --git a/security/security.c b/security/security.c
> > index 14d30fec8a00..47012c118536 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -30,7 +30,7 @@
> >  #include <linux/msg.h>
> >  #include <net/flow.h>
> >  
> > -#define MAX_LSM_EVM_XATTR	2
> > +#define MAX_LSM_EVM_XATTR	5
> >  
> >  /* How many LSMs were built into the kernel? */
> >  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
> > @@ -746,6 +746,29 @@ static int lsm_superblock_alloc(struct
> > super_block *sb)
> >  	RC;							\
> >  })
> >  
> > +#define call_int_hook_xattr(XATTRS, FUNC, IRC, ...) ({		
> > \
> > +	int RC = IRC;						\
> > +	int i = 0;						\
> > +	do {							\
> > +		struct security_hook_list *P;			\
> > +								\
> > +		hlist_for_each_entry(P, &security_hook_heads.FUNC,
> > list) { \
> > +			RC = P->hook.FUNC(__VA_ARGS__);		\
> > +			if (RC == -EOPNOTSUPP)			\
> > +				continue;			\
> > +			if (RC != 0 && RC != IRC)		\
> > +				break;				\
> > +			if (i >= MAX_LSM_EVM_XATTR) {		\
> > +				RC = -ENOMEM;			\
> > +				break;				\
> > +			}					\
> > +			XATTRS++;				\
> > +			i++;					\
> > +		}						\
> > +	} while (0);						\
> > +	RC;							\
> > +})
> > +
> >  /* Security operations */
> >  
> >  int security_binder_set_context_mgr(const struct cred *mgr)
> > @@ -1103,7 +1126,7 @@ int security_inode_init_security(struct inode
> > *inode, struct inode *dir,
> >  				     dir, qstr, NULL, NULL, NULL);
> >  	memset(new_xattrs, 0, sizeof(new_xattrs));
> >  	lsm_xattr = new_xattrs;
> > -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
> > dir, qstr,
> > +	ret = call_int_hook_xattr(lsm_xattr, inode_init_security,
> > -EOPNOTSUPP, inode, dir, qstr,
> >  						&lsm_xattr->name,
> >  						&lsm_xattr->value,
> >  						&lsm_xattr->value_len);
> > @@ -1111,7 +1134,7 @@ int security_inode_init_security(struct inode
> > *inode, struct inode *dir,
> >  		goto out;
> >  
> >  	evm_xattr = lsm_xattr + 1;
> > -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> > +	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);
> >  	if (ret)
> >  		goto out;
> >  	ret = initxattrs(inode, new_xattrs, fs_data);
> 

Best regards,

Nicolas Bouchinet

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Mimi Zohar]

Hi Nicolas,

On Fri, 2022-10-21 at 15:47 +0200, Nicolas Bouchinet wrote:
> Hi Mimi,
> 
> Thanks for the IMA/EVM project which I enjoy very much.
> 
> On Thu, Oct 20, 2022 at 03:51:38PM -0400, Mimi Zohar wrote:
> > On Thu, 2022-10-20 at 15:55 +0200, Nicolas Bouchinet wrote:
> > > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > > 
> > > Fixes a NULL pointer dereference occuring in the
> > > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > > triggered if a `inode_init_security` hook returns 0 without initializing
> > > the given `struct xattr` fields (which is the case of BPF) and if no
> > > other LSM overrides thoses fields after. This also leads to memory
> > > leaks.
> > > 
> > > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > > `new_xattrs` array with every called hook xattr values.
> > > 
> > > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > > entry of the array contrary to `evm_init_hmac`.
> >   
> > Only EVM portable digital signatures include all of the protected
> > xattrs.   Refer to commit 8c7a703ec978 ("evm: Verify portable
> > signatures against all protected xattrs").
> > 
> Sorry, maybe I was not clear enough, the proposed patch does not change the
> set of the protected security xattrs initialized by the LSMs and processed by EVM.
> 
> As I explained to Paul, based on my understanding, the `security_inode_init_security()`
> hook is supposed to initialize every hooked LSM security xattr and next,
> if evm is enabled, protect them using a HMAC algorithm.
> However, in the current implementation, the use of the `call_int_hook()` macro by
> `security_inode_init_security()` overwrites the previously initialized xattr for
> each iteration of the `hlist_for_each_entry()` loop.
> 
> I have noticed that more than one LSM may initialize a security xattr at a time,
> eg. SELinux + BPF.

Does BPF have a security xattr and, if so, does it need to be
protected?   It would need to be defined and included in the list of
evm_config_xattrnames[].  If it doesn't define a security bpf xattr,
then bpf should not be on the security_inode_init_security() hook.  (I
assume Roberto's patch is going in this direction.)

Before the EVM hmac is updated, the existing EVM hmac is verified.  I
would be concerned if bpf defined a protected security xattr.   Could
the same guarantees, that security.evm isn't updated without first
being verified, be enforced with bpf?

> 
> IMHO my supplementary `evm_init_hmacs()` function name is a bit confusing, I would
> enjoy if you have a better proposition. Note that `evm_init_hmacs()` have the same
> behavior as `evm_init_hmac()` if only one security xattr is given as a parameter.

I'm missing something here.  As evm_inode_init_security() is the only
caller of evm_init_hmac(), why is a new function defined instead of
updating the existing one?   If there is a valid reason, then one
function should be a wrapper for the other.

> > > 
> > > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> > 
> > Won't this break existing EVM hmac usage?
> I might be wrong, but as far as I understand it, the only working condition for
> EVM now is when only one security xattr is involved, otherwise there will have
> a mismatch between the initialization and the verification.
> Indeed, the verification takes into account every security xattr written in its
> refering dentry.

Agreed, independently as to whether BPF defines a security xattr, if
two LSMs initialize security xattrs, then this change is needed.  Are
there any other examples?

(nit: I understand the line size has generally been relaxed, but for
IMA/EVM I would prefer it to be remain as 80 chars.)

Mimi

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi !

On Mon, Oct 24, 2022 at 12:35:52PM -0400, Mimi Zohar wrote:
> Hi Nicolas,
> 
> On Fri, 2022-10-21 at 15:47 +0200, Nicolas Bouchinet wrote:
> > Hi Mimi,
> > 
> > Thanks for the IMA/EVM project which I enjoy very much.
> > 
> > On Thu, Oct 20, 2022 at 03:51:38PM -0400, Mimi Zohar wrote:
> > > On Thu, 2022-10-20 at 15:55 +0200, Nicolas Bouchinet wrote:
> > > > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > > > 
> > > > Fixes a NULL pointer dereference occuring in the
> > > > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > > > triggered if a `inode_init_security` hook returns 0 without initializing
> > > > the given `struct xattr` fields (which is the case of BPF) and if no
> > > > other LSM overrides thoses fields after. This also leads to memory
> > > > leaks.
> > > > 
> > > > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > > > `new_xattrs` array with every called hook xattr values.
> > > > 
> > > > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > > > entry of the array contrary to `evm_init_hmac`.
> > >   
> > > Only EVM portable digital signatures include all of the protected
> > > xattrs.   Refer to commit 8c7a703ec978 ("evm: Verify portable
> > > signatures against all protected xattrs").
> > > 
> > Sorry, maybe I was not clear enough, the proposed patch does not change the
> > set of the protected security xattrs initialized by the LSMs and processed by EVM.
> > 
> > As I explained to Paul, based on my understanding, the `security_inode_init_security()`
> > hook is supposed to initialize every hooked LSM security xattr and next,
> > if evm is enabled, protect them using a HMAC algorithm.
> > However, in the current implementation, the use of the `call_int_hook()` macro by
> > `security_inode_init_security()` overwrites the previously initialized xattr for
> > each iteration of the `hlist_for_each_entry()` loop.
> > 
> > I have noticed that more than one LSM may initialize a security xattr at a time,
> > eg. SELinux + BPF.
> 
> Does BPF have a security xattr and, if so, does it need to be
> protected?   It would need to be defined and included in the list of
> evm_config_xattrnames[].  If it doesn't define a security bpf xattr,
> then bpf should not be on the security_inode_init_security() hook.  (I
> assume Roberto's patch is going in this direction.)
> 
> Before the EVM hmac is updated, the existing EVM hmac is verified.  I
> would be concerned if bpf defined a protected security xattr.   Could
> the same guarantees, that security.evm isn't updated without first
> being verified, be enforced with bpf?
> 

I am not that comfortable with BPF programs but based on what Alexei Starovoitov pointed out here
https://lore.kernel.org/bpf/20221021164626.3729012-1-roberto.sassu@huaweicloud.com
BPF should not be able to write into the xattrs pointers. And thus shouldn't be included
in `evm_config_xattrnames[]`.
> > 
> > IMHO my supplementary `evm_init_hmacs()` function name is a bit confusing, I would
> > enjoy if you have a better proposition. Note that `evm_init_hmacs()` have the same
> > behavior as `evm_init_hmac()` if only one security xattr is given as a parameter.
> 
> I'm missing something here.  As evm_inode_init_security() is the only
> caller of evm_init_hmac(), why is a new function defined instead of
> updating the existing one?   If there is a valid reason, then one
> function should be a wrapper for the other.
> 

There is no valid reasons, I was just unsure about replacing existing functions, will update it.
> > > > 
> > > > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> > > 
> > > Won't this break existing EVM hmac usage?
> > I might be wrong, but as far as I understand it, the only working condition for
> > EVM now is when only one security xattr is involved, otherwise there will have
> > a mismatch between the initialization and the verification.
> > Indeed, the verification takes into account every security xattr written in its
> > refering dentry.
> 
> Agreed, independently as to whether BPF defines a security xattr, if
> two LSMs initialize security xattrs, then this change is needed.  Are
> there any other examples?

I think that in its current state the kernel cannot load two LSM capable of xattr
initialization as they are all defined with the `LSM_FLAG_EXCLUSIVE` flag set.
But I may be unaware of other LSM in development stage.
> 
> (nit: I understand the line size has generally been relaxed, but for
> IMA/EVM I would prefer it to be remain as 80 chars.)
> 

No problem, will change it !
> Mimi
> 

I'll take time to run few tests with BPF and send a patch v3 with new changes.

Regards,

Nicolas Bouchinet

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Mimi Zohar]

On Tue, 2022-10-25 at 15:33 +0200, Nicolas Bouchinet wrote:
> > Agreed, independently as to whether BPF defines a security xattr, if
> > two LSMs initialize security xattrs, then this change is needed.  Are
> > there any other examples?
> 
> I think that in its current state the kernel cannot load two LSM capable of xattr
> initialization as they are all defined with the `LSM_FLAG_EXCLUSIVE` flag set.
> But I may be unaware of other LSM in development stage.

Casey, Paul, can we get confirmation on this?

> > 
> > (nit: I understand the line size has generally been relaxed, but for
> > IMA/EVM I would prefer it to be remain as 80 chars.)
> > 
> 
> No problem, will change it !
> 
> I'll take time to run few tests with BPF and send a patch v3 with new changes.

Since Roberto's patches will address the BPF bug(s), is this a fix for
a real bug or a possbile future one.   Cc'ing stable might not be
necessary.

thanks,

Mimi

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Mimi Zohar]

On Tue, 2022-10-25 at 10:21 -0400, Mimi Zohar wrote:
> On Tue, 2022-10-25 at 15:33 +0200, Nicolas Bouchinet wrote:
> > > Agreed, independently as to whether BPF defines a security xattr, if
> > > two LSMs initialize security xattrs, then this change is needed.  Are
> > > there any other examples?
> > 
> > I think that in its current state the kernel cannot load two LSM capable of xattr
> > initialization as they are all defined with the `LSM_FLAG_EXCLUSIVE` flag set.
> > But I may be unaware of other LSM in development stage.
> 
> Casey, Paul, can we get confirmation on this?
> 
> > > 
> > > (nit: I understand the line size has generally been relaxed, but for
> > > IMA/EVM I would prefer it to be remain as 80 chars.)
> > > 
> > 
> > No problem, will change it !
> > 
> > I'll take time to run few tests with BPF and send a patch v3 with new changes.
> 
> Since Roberto's patches will address the BPF bug(s), is this a fix for
> a real bug or a possbile future one.   Cc'ing stable might not be
> necessary.

And the patch description will need to be updated accordingly.

thanks,

Mimi

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Casey Schaufler]

On 10/25/2022 7:21 AM, Mimi Zohar wrote:
> On Tue, 2022-10-25 at 15:33 +0200, Nicolas Bouchinet wrote:
>>> Agreed, independently as to whether BPF defines a security xattr, if
>>> two LSMs initialize security xattrs, then this change is needed.  Are
>>> there any other examples?
>> I think that in its current state the kernel cannot load two LSM capable of xattr
>> initialization as they are all defined with the `LSM_FLAG_EXCLUSIVE` flag set.
>> But I may be unaware of other LSM in development stage.
> Casey, Paul, can we get confirmation on this?

I'm working really hard to eliminate LSM_FLAG_EXCLUSIVE. Dealing with
multiple security modules initializing security xattrs has been in the
stacking patch sets that have been in review for years now. So no,
you can't wave the problem away by pointing at LSM_FLAG_EXCLUSIVE.

>>> (nit: I understand the line size has generally been relaxed, but for
>>> IMA/EVM I would prefer it to be remain as 80 chars.)
>>>
>> No problem, will change it !
>>
>> I'll take time to run few tests with BPF and send a patch v3 with new changes.
> Since Roberto's patches will address the BPF bug(s), is this a fix for
> a real bug or a possbile future one.   Cc'ing stable might not be
> necessary.
>
> thanks,
>
> Mimi
>

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Mimi Zohar]

On Tue, 2022-10-25 at 08:06 -0700, Casey Schaufler wrote:
> On 10/25/2022 7:21 AM, Mimi Zohar wrote:
> > On Tue, 2022-10-25 at 15:33 +0200, Nicolas Bouchinet wrote:
> >>> Agreed, independently as to whether BPF defines a security xattr, if
> >>> two LSMs initialize security xattrs, then this change is needed.  Are
> >>> there any other examples?
> >> I think that in its current state the kernel cannot load two LSM capable of xattr
> >> initialization as they are all defined with the `LSM_FLAG_EXCLUSIVE` flag set.
> >> But I may be unaware of other LSM in development stage.
> > Casey, Paul, can we get confirmation on this?
> 
> I'm working really hard to eliminate LSM_FLAG_EXCLUSIVE. Dealing with
> multiple security modules initializing security xattrs has been in the
> stacking patch sets that have been in review for years now. So no,
> you can't wave the problem away by pointing at LSM_FLAG_EXCLUSIVE.

Please note that the original problem being addressed by this patch
will be addressed by Roberto's BPF patch.   The question here was
whether this addresses an existing bug, other than BPF, or a future
one, and whether it needs to be backported.

From your response, initializing multiple security xattrs is not an
issue at the moment so it doesn't need to be backported.  Whether this
patch should be upstreamed with the LSM stacking patch set is a
separate question.

> 
> >>> (nit: I understand the line size has generally been relaxed, but for
> >>> IMA/EVM I would prefer it to be remain as 80 chars.)
> >>>
> >> No problem, will change it !
> >>
> >> I'll take time to run few tests with BPF and send a patch v3 with new changes.
> > Since Roberto's patches will address the BPF bug(s), is this a fix for
> > a real bug or a possbile future one.   Cc'ing stable might not be
> > necessary.

-- 
thanks,

Mimi

Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi Mimi,

On Tue, Oct 25, 2022 at 11:58:40AM -0400, Mimi Zohar wrote:
> On Tue, 2022-10-25 at 08:06 -0700, Casey Schaufler wrote:
> > On 10/25/2022 7:21 AM, Mimi Zohar wrote:
> > > On Tue, 2022-10-25 at 15:33 +0200, Nicolas Bouchinet wrote:
> > >>> Agreed, independently as to whether BPF defines a security xattr, if
> > >>> two LSMs initialize security xattrs, then this change is needed.  Are
> > >>> there any other examples?
> > >> I think that in its current state the kernel cannot load two LSM capable of xattr
> > >> initialization as they are all defined with the `LSM_FLAG_EXCLUSIVE` flag set.
> > >> But I may be unaware of other LSM in development stage.
> > > Casey, Paul, can we get confirmation on this?
> > 
> > I'm working really hard to eliminate LSM_FLAG_EXCLUSIVE. Dealing with
> > multiple security modules initializing security xattrs has been in the
> > stacking patch sets that have been in review for years now. So no,
> > you can't wave the problem away by pointing at LSM_FLAG_EXCLUSIVE.
> 
> Please note that the original problem being addressed by this patch
> will be addressed by Roberto's BPF patch.   The question here was
> whether this addresses an existing bug, other than BPF, or a future
> one, and whether it needs to be backported.
> 
Should I split the NULL pointer dereference fix in a separated patch for EVM ?
> From your response, initializing multiple security xattrs is not an
> issue at the moment so it doesn't need to be backported.  Whether this
> patch should be upstreamed with the LSM stacking patch set is a
> separate question.
> 
> > 
> > >>> (nit: I understand the line size has generally been relaxed, but for
> > >>> IMA/EVM I would prefer it to be remain as 80 chars.)
> > >>>
> > >> No problem, will change it !
> > >>
> > >> I'll take time to run few tests with BPF and send a patch v3 with new changes.
> > > Since Roberto's patches will address the BPF bug(s), is this a fix for
> > > a real bug or a possbile future one.   Cc'ing stable might not be
> > > necessary.
Ok, will remove stable.

Thanks,

Nicolas Bouchinet