[PATCH v4] evm: Correct inode_init_security hooks behaviors

[PATCH v4] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

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

Fixes a NULL pointer dereference occurring 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.

The `call_int_hook_xattr` macro has been inlined into the
`security_inode_init_security` hook in order to check hooks return
values and skip ones who doesn't init `xattrs`.

Modify `evm_init_hmac` function to init the EVM hmac using every
entry of the given xattr array.

The `MAX_LSM_EVM_XATTR` value is now based on the security modules
compiled in, 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: roberto.sassu@huaweicloud.com
Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
---
Changes since v3:
https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/

* Fixes compilation error reported by the kernel test robot.
---
 include/linux/lsm_hook_defs.h       |  2 +-
 include/linux/lsm_hooks.h           |  4 ++--
 security/integrity/evm/evm.h        |  1 +
 security/integrity/evm/evm_crypto.c |  9 +++++++--
 security/integrity/evm/evm_main.c   |  7 ++++---
 security/security.c                 | 31 ++++++++++++++++++++++-------
 6 files changed, 39 insertions(+), 15 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..6d9628ca7c24 100644
--- a/security/integrity/evm/evm.h
+++ b/security/integrity/evm/evm.h
@@ -61,5 +61,6 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
 int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
 		  char *hmac_val);
 int evm_init_secfs(void);
+int evm_protected_xattr(const char *req_xattr_name);
 
 #endif
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 708de9656bbd..06639f3cfb38 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -385,7 +385,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 	return rc;
 }
 
-int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
+int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattrs,
 		  char *hmac_val)
 {
 	struct shash_desc *desc;
@@ -396,7 +396,12 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
 		return PTR_ERR(desc);
 	}
 
-	crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
+	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;
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2e6fb6e2ffd2..0420453a80e8 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);
 }
@@ -841,8 +843,7 @@ int evm_inode_init_security(struct inode *inode,
 	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);
diff --git a/security/security.c b/security/security.c
index 14d30fec8a00..79524f8734f1 100644
--- a/security/security.c
+++ b/security/security.c
@@ -30,7 +30,11 @@
 #include <linux/msg.h>
 #include <net/flow.h>
 
-#define MAX_LSM_EVM_XATTR	2
+#define MAX_LSM_EVM_XATTR                                \
+	((IS_ENABLED(CONFIG_EVM) ? 1 : 0) +              \
+	 (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
+	 (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 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)
@@ -1091,9 +1095,11 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
 				 const struct qstr *qstr,
 				 const initxattrs initxattrs, void *fs_data)
 {
+	int i = 0;
+	int ret = -EOPNOTSUPP;
 	struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
 	struct xattr *lsm_xattr, *evm_xattr, *xattr;
-	int ret;
+	struct security_hook_list *hook_ptr;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return 0;
@@ -1103,15 +1109,26 @@ 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,
-						&lsm_xattr->name,
-						&lsm_xattr->value,
-						&lsm_xattr->value_len);
+	hlist_for_each_entry(hook_ptr, &security_hook_heads.inode_init_security,
+			     list) {
+		ret = hook_ptr->hook.inode_init_security(inode, dir, qstr,
+				&lsm_xattr->name,
+				&lsm_xattr->value,
+				&lsm_xattr->value_len);
+		if (ret == -EOPNOTSUPP)
+			continue;
+		if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))
+			ret = -ENOMEM;
+		if (ret != 0)
+			break;
+		lsm_xattr++;
+		i++;
+	}
 	if (ret)
 		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 v4] evm: Correct inode_init_security hooks behaviors

[Roberto Sassu]

On Wed, 2022-10-26 at 16:30 +0200, Nicolas Bouchinet wrote:
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> 
> Fixes a NULL pointer dereference occurring 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.
> 
> The `call_int_hook_xattr` macro has been inlined into the
> `security_inode_init_security` hook in order to check hooks return
> values and skip ones who doesn't init `xattrs`.
> 
> Modify `evm_init_hmac` function to init the EVM hmac using every
> entry of the given xattr array.
> 
> The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> compiled in, 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: roberto.sassu@huaweicloud.com
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
> Changes since v3:
> https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/
> 
> * Fixes compilation error reported by the kernel test robot.
> ---
>  include/linux/lsm_hook_defs.h       |  2 +-
>  include/linux/lsm_hooks.h           |  4 ++--
>  security/integrity/evm/evm.h        |  1 +
>  security/integrity/evm/evm_crypto.c |  9 +++++++--
>  security/integrity/evm/evm_main.c   |  7 ++++---
>  security/security.c                 | 31 ++++++++++++++++++++++-------
>  6 files changed, 39 insertions(+), 15 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.

Please also update the description of @name as well (remove allocated).

>   *	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..6d9628ca7c24 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -61,5 +61,6 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
>  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
>  		  char *hmac_val);
>  int evm_init_secfs(void);
> +int evm_protected_xattr(const char *req_xattr_name);
>  
>  #endif
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index 708de9656bbd..06639f3cfb38 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -385,7 +385,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  	return rc;
>  }
>  
> -int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
> +int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattrs,
>  		  char *hmac_val)
>  {
>  	struct shash_desc *desc;
> @@ -396,7 +396,12 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
>  		return PTR_ERR(desc);
>  	}
>  
> -	crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
> +	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;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 2e6fb6e2ffd2..0420453a80e8 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;

Remove, and use the check below.

>  	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);
>  }
> @@ -841,8 +843,7 @@ int evm_inode_init_security(struct inode *inode,
>  	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);
> diff --git a/security/security.c b/security/security.c
> index 14d30fec8a00..79524f8734f1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,7 +30,11 @@
>  #include <linux/msg.h>
>  #include <net/flow.h>
>  
> -#define MAX_LSM_EVM_XATTR	2
> +#define MAX_LSM_EVM_XATTR                                \
> +	((IS_ENABLED(CONFIG_EVM) ? 1 : 0) +              \
> +	 (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> +	 (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 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)
> @@ -1091,9 +1095,11 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  				 const struct qstr *qstr,
>  				 const initxattrs initxattrs, void *fs_data)
>  {
> +	int i = 0;
> +	int ret = -EOPNOTSUPP;
>  	struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>  	struct xattr *lsm_xattr, *evm_xattr, *xattr;
> -	int ret;
> +	struct security_hook_list *hook_ptr;
>  
>  	if (unlikely(IS_PRIVATE(inode)))
>  		return 0;
> @@ -1103,15 +1109,26 @@ 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,
> -						&lsm_xattr->name,
> -						&lsm_xattr->value,
> -						&lsm_xattr->value_len);
> +	hlist_for_each_entry(hook_ptr, &security_hook_heads.inode_init_security,
> +			     list) {
> +		ret = hook_ptr->hook.inode_init_security(inode, dir, qstr,
> +				&lsm_xattr->name,
> +				&lsm_xattr->value,
> +				&lsm_xattr->value_len);
> +		if (ret == -EOPNOTSUPP)
> +			continue;

This does not work properly. Suppose that you have an LSM with xattr
and another without. The final ret will be -EOPNOTSUPP. Instead declare
new_xattrs_set boolean, and set to true if ret = 0. After the loop,
check the boolean instead of ret. If ret != 0 goto out.

> +		if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))
> +			ret = -ENOMEM;
> +		if (ret != 0)
> +			break;

We can check here if the LSM behaved properly, i.e. it set the xattr
name and value:

if (WARN_ON_ONCE(!lsm_xattr->name || !lsm_xattr->value)) {
	ret = -ENOMEM;
	goto out;
}

> +		lsm_xattr++;
> +		i++;
> +	}
>  	if (ret)
>  		goto out;
>  
>  	evm_xattr = lsm_xattr + 1;

It should be:

	evm_xattr = lsm_xattr;

You incremented lsm_xattr already, after the LSMs set their xattr.

Once you complete the changes, I will send a patch set including your
patch with some more patches.

Roberto

> -	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 v4] evm: Correct inode_init_security hooks behaviors

[Roberto Sassu]

On Thu, 2022-11-03 at 16:27 +0100, Roberto Sassu wrote:
> On Wed, 2022-10-26 at 16:30 +0200, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > Fixes a NULL pointer dereference occurring 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.
> > 
> > The `call_int_hook_xattr` macro has been inlined into the
> > `security_inode_init_security` hook in order to check hooks return
> > values and skip ones who doesn't init `xattrs`.
> > 
> > Modify `evm_init_hmac` function to init the EVM hmac using every
> > entry of the given xattr array.
> > 
> > The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> > compiled in, 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: roberto.sassu@huaweicloud.com
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> > Changes since v3:
> > https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/
> > 
> > * Fixes compilation error reported by the kernel test robot.
> > ---
> >  include/linux/lsm_hook_defs.h       |  2 +-
> >  include/linux/lsm_hooks.h           |  4 ++--
> >  security/integrity/evm/evm.h        |  1 +
> >  security/integrity/evm/evm_crypto.c |  9 +++++++--
> >  security/integrity/evm/evm_main.c   |  7 ++++---
> >  security/security.c                 | 31 ++++++++++++++++++++++-------
> >  6 files changed, 39 insertions(+), 15 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.
> 
> Please also update the description of @name as well (remove allocated).

While you update the patch, I worked on the other patches: reiserfs
fixes, if we want still to apply them; expand the call_int_hook() loop
also for security_old_inode_init_security() to have consistent behavior
across all filesystems.

The patches are available here:

https://github.com/robertosassu/linux/tree/evm-multiple-lsms-nicolas-v1-devel-v6

Other than Github Actions related patches, there is also TestLSM, which
I developed to ensure that xattrs are correctly created.

I also adapted the IMA/EVM tests, which are available here:

https://github.com/robertosassu/ima-evm-utils/tree/evm-multiple-lsms-nicolas-v1-devel-v6

Nicolas, if you want to test the new patch locally, build the UML
kernel with:

make ARCH=um -j$(nproc)

Build ima-evm-utils, and copy linux and certs/signing_key.pem from the
kernel source directory to the ima-evm-utils directory. Then, run:

tests/evm_multiple_lsms.test

It basically runs the UML kernel with different combinations of LSMs
(some providing an xattr, some not) and compares the HMAC calculated by
EVM in the kernel with the HMAC calculated by evmctl in user space.

Roberto

Re: [PATCH v4] evm: Correct inode_init_security hooks behaviors

[Paul Moore]

On Wed, Oct 26, 2022 at 10:30 AM Nicolas Bouchinet
<nicolas.bouchinet@clip-os.org> wrote:
>
> From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
>
> Fixes a NULL pointer dereference occurring 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.
>
> The `call_int_hook_xattr` macro has been inlined into the
> `security_inode_init_security` hook in order to check hooks return
> values and skip ones who doesn't init `xattrs`.
>
> Modify `evm_init_hmac` function to init the EVM hmac using every
> entry of the given xattr array.
>
> The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> compiled in, 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: roberto.sassu@huaweicloud.com
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> ---
> Changes since v3:
> https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/
>
> * Fixes compilation error reported by the kernel test robot.
> ---
>  include/linux/lsm_hook_defs.h       |  2 +-
>  include/linux/lsm_hooks.h           |  4 ++--
>  security/integrity/evm/evm.h        |  1 +
>  security/integrity/evm/evm_crypto.c |  9 +++++++--
>  security/integrity/evm/evm_main.c   |  7 ++++---
>  security/security.c                 | 31 ++++++++++++++++++++++-------
>  6 files changed, 39 insertions(+), 15 deletions(-)

...

> diff --git a/security/security.c b/security/security.c
> index 14d30fec8a00..79524f8734f1 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,7 +30,11 @@
>  #include <linux/msg.h>
>  #include <net/flow.h>
>
> -#define MAX_LSM_EVM_XATTR      2
> +#define MAX_LSM_EVM_XATTR                                \
> +       ((IS_ENABLED(CONFIG_EVM) ? 1 : 0) +              \
> +        (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> +        (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) +   \
> +        (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))

...

> @@ -1091,9 +1095,11 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>                                  const struct qstr *qstr,
>                                  const initxattrs initxattrs, void *fs_data)
>  {
> +       int i = 0;
> +       int ret = -EOPNOTSUPP;
>         struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>         struct xattr *lsm_xattr, *evm_xattr, *xattr;
> -       int ret;
> +       struct security_hook_list *hook_ptr;
>
>         if (unlikely(IS_PRIVATE(inode)))
>                 return 0;
> @@ -1103,15 +1109,26 @@ 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,
> -                                               &lsm_xattr->name,
> -                                               &lsm_xattr->value,
> -                                               &lsm_xattr->value_len);
> +       hlist_for_each_entry(hook_ptr, &security_hook_heads.inode_init_security,
> +                            list) {
> +               ret = hook_ptr->hook.inode_init_security(inode, dir, qstr,
> +                               &lsm_xattr->name,
> +                               &lsm_xattr->value,
> +                               &lsm_xattr->value_len);
> +               if (ret == -EOPNOTSUPP)
> +                       continue;
> +               if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))
> +                       ret = -ENOMEM;

It would really like to see us get rid of the MAX_LSM_EVM_XATTR macro
and determine the array size similar to what we do with the security
blob sizes.  The macro definition is a kludgy hack that is bound to
get out of sync at some point and this extra checking inside the hook
is something we should work to remove.

> +               if (ret != 0)
> +                       break;
> +               lsm_xattr++;
> +               i++;
> +       }
>         if (ret)
>                 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

-- 
paul-moore.com

Re: [PATCH v4] evm: Correct inode_init_security hooks behaviors

[Roberto Sassu]

On Sat, 2022-11-05 at 07:06 -0400, Paul Moore wrote:
> On Wed, Oct 26, 2022 at 10:30 AM Nicolas Bouchinet
> <nicolas.bouchinet@clip-os.org> wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > Fixes a NULL pointer dereference occurring 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.
> > 
> > The `call_int_hook_xattr` macro has been inlined into the
> > `security_inode_init_security` hook in order to check hooks return
> > values and skip ones who doesn't init `xattrs`.
> > 
> > Modify `evm_init_hmac` function to init the EVM hmac using every
> > entry of the given xattr array.
> > 
> > The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> > compiled in, 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: roberto.sassu@huaweicloud.com
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> > Changes since v3:
> > https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/
> > 
> > * Fixes compilation error reported by the kernel test robot.
> > ---
> >  include/linux/lsm_hook_defs.h       |  2 +-
> >  include/linux/lsm_hooks.h           |  4 ++--
> >  security/integrity/evm/evm.h        |  1 +
> >  security/integrity/evm/evm_crypto.c |  9 +++++++--
> >  security/integrity/evm/evm_main.c   |  7 ++++---
> >  security/security.c                 | 31 ++++++++++++++++++++++-------
> >  6 files changed, 39 insertions(+), 15 deletions(-)
> 
> ...
> 
> > diff --git a/security/security.c b/security/security.c
> > index 14d30fec8a00..79524f8734f1 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -30,7 +30,11 @@
> >  #include <linux/msg.h>
> >  #include <net/flow.h>
> > 
> > -#define MAX_LSM_EVM_XATTR      2
> > +#define MAX_LSM_EVM_XATTR                                \
> > +       ((IS_ENABLED(CONFIG_EVM) ? 1 : 0) +              \
> > +        (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> > +        (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) +   \
> > +        (IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0))
> 
> ...
> 
> > @@ -1091,9 +1095,11 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> >                                  const struct qstr *qstr,
> >                                  const initxattrs initxattrs, void *fs_data)
> >  {
> > +       int i = 0;
> > +       int ret = -EOPNOTSUPP;
> >         struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
> >         struct xattr *lsm_xattr, *evm_xattr, *xattr;
> > -       int ret;
> > +       struct security_hook_list *hook_ptr;
> > 
> >         if (unlikely(IS_PRIVATE(inode)))
> >                 return 0;
> > @@ -1103,15 +1109,26 @@ 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,
> > -                                               &lsm_xattr->name,
> > -                                               &lsm_xattr->value,
> > -                                               &lsm_xattr->value_len);
> > +       hlist_for_each_entry(hook_ptr, &security_hook_heads.inode_init_security,
> > +                            list) {
> > +               ret = hook_ptr->hook.inode_init_security(inode, dir, qstr,
> > +                               &lsm_xattr->name,
> > +                               &lsm_xattr->value,
> > +                               &lsm_xattr->value_len);
> > +               if (ret == -EOPNOTSUPP)
> > +                       continue;
> > +               if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))
> > +                       ret = -ENOMEM;
> 
> It would really like to see us get rid of the MAX_LSM_EVM_XATTR macro
> and determine the array size similar to what we do with the security
> blob sizes.  The macro definition is a kludgy hack that is bound to
> get out of sync at some point and this extra checking inside the hook
> is something we should work to remove.

In this case, I already implemented this, as it was originally
suggested by Casey. I will resend this:

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

with few minor tweaks.

Roberto

> > +               if (ret != 0)
> > +                       break;
> > +               lsm_xattr++;
> > +               i++;
> > +       }
> >         if (ret)
> >                 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 v4] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

Hi Roberto, sorry for the late reply.

On Thu, Nov 03, 2022 at 04:27:51PM +0100, Roberto Sassu wrote:
> On Wed, 2022-10-26 at 16:30 +0200, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > Fixes a NULL pointer dereference occurring 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.
> > 
> > The `call_int_hook_xattr` macro has been inlined into the
> > `security_inode_init_security` hook in order to check hooks return
> > values and skip ones who doesn't init `xattrs`.
> > 
> > Modify `evm_init_hmac` function to init the EVM hmac using every
> > entry of the given xattr array.
> > 
> > The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> > compiled in, 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: roberto.sassu@huaweicloud.com
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> > Changes since v3:
> > https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/
> > 
> > * Fixes compilation error reported by the kernel test robot.
> > ---
> >  include/linux/lsm_hook_defs.h       |  2 +-
> >  include/linux/lsm_hooks.h           |  4 ++--
> >  security/integrity/evm/evm.h        |  1 +
> >  security/integrity/evm/evm_crypto.c |  9 +++++++--
> >  security/integrity/evm/evm_main.c   |  7 ++++---
> >  security/security.c                 | 31 ++++++++++++++++++++++-------
> >  6 files changed, 39 insertions(+), 15 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.
> 
> Please also update the description of @name as well (remove allocated).
ACK, will do 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..6d9628ca7c24 100644
> > --- a/security/integrity/evm/evm.h
> > +++ b/security/integrity/evm/evm.h
> > @@ -61,5 +61,6 @@ int evm_calc_hash(struct dentry *dentry, const char *req_xattr_name,
> >  int evm_init_hmac(struct inode *inode, const struct xattr *xattr,
> >  		  char *hmac_val);
> >  int evm_init_secfs(void);
> > +int evm_protected_xattr(const char *req_xattr_name);
> >  
> >  #endif
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index 708de9656bbd..06639f3cfb38 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -385,7 +385,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> >  	return rc;
> >  }
> >  
> > -int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
> > +int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattrs,
> >  		  char *hmac_val)
> >  {
> >  	struct shash_desc *desc;
> > @@ -396,7 +396,12 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr,
> >  		return PTR_ERR(desc);
> >  	}
> >  
> > -	crypto_shash_update(desc, lsm_xattr->value, lsm_xattr->value_len);
> > +	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;
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 2e6fb6e2ffd2..0420453a80e8 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;
> 
> Remove, and use the check below.
If I understood it well, this patchset will not be backported. Wouldn't it be necessary to make this check here
for backports ?
> 
> >  	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);
> >  }
> > @@ -841,8 +843,7 @@ int evm_inode_init_security(struct inode *inode,
> >  	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);
> > diff --git a/security/security.c b/security/security.c
> > index 14d30fec8a00..79524f8734f1 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -30,7 +30,11 @@
> >  #include <linux/msg.h>
> >  #include <net/flow.h>
> >  
> > -#define MAX_LSM_EVM_XATTR	2
> > +#define MAX_LSM_EVM_XATTR                                \
> > +	((IS_ENABLED(CONFIG_EVM) ? 1 : 0) +              \
> > +	 (IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0) + \
> > +	 (IS_ENABLED(CONFIG_SECURITY_SMACK) ? 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)
> > @@ -1091,9 +1095,11 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
> >  				 const struct qstr *qstr,
> >  				 const initxattrs initxattrs, void *fs_data)
> >  {
> > +	int i = 0;
> > +	int ret = -EOPNOTSUPP;
> >  	struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
> >  	struct xattr *lsm_xattr, *evm_xattr, *xattr;
> > -	int ret;
> > +	struct security_hook_list *hook_ptr;
> >  
> >  	if (unlikely(IS_PRIVATE(inode)))
> >  		return 0;
> > @@ -1103,15 +1109,26 @@ 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,
> > -						&lsm_xattr->name,
> > -						&lsm_xattr->value,
> > -						&lsm_xattr->value_len);
> > +	hlist_for_each_entry(hook_ptr, &security_hook_heads.inode_init_security,
> > +			     list) {
> > +		ret = hook_ptr->hook.inode_init_security(inode, dir, qstr,
> > +				&lsm_xattr->name,
> > +				&lsm_xattr->value,
> > +				&lsm_xattr->value_len);
> > +		if (ret == -EOPNOTSUPP)
> > +			continue;
> 
> This does not work properly. Suppose that you have an LSM with xattr
> and another without. The final ret will be -EOPNOTSUPP. Instead declare
> new_xattrs_set boolean, and set to true if ret = 0. After the loop,
> check the boolean instead of ret. If ret != 0 goto out.
> 
Your right, will change it.

> > +		if (WARN_ON_ONCE(i >= MAX_LSM_EVM_XATTR))
> > +			ret = -ENOMEM;
> > +		if (ret != 0)
> > +			break;
> 
> We can check here if the LSM behaved properly, i.e. it set the xattr
> name and value:
> 
> if (WARN_ON_ONCE(!lsm_xattr->name || !lsm_xattr->value)) {
> 	ret = -ENOMEM;
> 	goto out;
I'm OK with this, thanks, will change it !

> }
> 
> > +		lsm_xattr++;
> > +		i++;
> > +	}
> >  	if (ret)
> >  		goto out;
> >  
> >  	evm_xattr = lsm_xattr + 1;
> 
> It should be:
> 
> 	evm_xattr = lsm_xattr;
> 
> You incremented lsm_xattr already, after the LSMs set their xattr.

`lsm_xattr` is indeed already incremented, will patch this.

> 
> Once you complete the changes, I will send a patch set including your
> patch with some more patches.
> 
> Roberto
> 
> > -	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);
> 

Thank's a lot for your review,

Nicolas Bouchinet

Re: [PATCH v4] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet]

On Fri, Nov 04, 2022 at 02:43:25PM +0100, Roberto Sassu wrote:
> On Thu, 2022-11-03 at 16:27 +0100, Roberto Sassu wrote:
> > On Wed, 2022-10-26 at 16:30 +0200, Nicolas Bouchinet wrote:
> > > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > > 
> > > Fixes a NULL pointer dereference occurring 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.
> > > 
> > > The `call_int_hook_xattr` macro has been inlined into the
> > > `security_inode_init_security` hook in order to check hooks return
> > > values and skip ones who doesn't init `xattrs`.
> > > 
> > > Modify `evm_init_hmac` function to init the EVM hmac using every
> > > entry of the given xattr array.
> > > 
> > > The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> > > compiled in, 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: roberto.sassu@huaweicloud.com
> > > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > > ---
> > > Changes since v3:
> > > https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/
> > > 
> > > * Fixes compilation error reported by the kernel test robot.
> > > ---
> > >  include/linux/lsm_hook_defs.h       |  2 +-
> > >  include/linux/lsm_hooks.h           |  4 ++--
> > >  security/integrity/evm/evm.h        |  1 +
> > >  security/integrity/evm/evm_crypto.c |  9 +++++++--
> > >  security/integrity/evm/evm_main.c   |  7 ++++---
> > >  security/security.c                 | 31 ++++++++++++++++++++++-------
> > >  6 files changed, 39 insertions(+), 15 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.
> > 
> > Please also update the description of @name as well (remove allocated).
> 
> While you update the patch, I worked on the other patches: reiserfs
> fixes, if we want still to apply them; expand the call_int_hook() loop
> also for security_old_inode_init_security() to have consistent behavior
> across all filesystems.

Thank's I didn't had time to read the patch, will do it.
> 
> The patches are available here:
> 
> https://github.com/robertosassu/linux/tree/evm-multiple-lsms-nicolas-v1-devel-v6
> 
> Other than Github Actions related patches, there is also TestLSM, which
> I developed to ensure that xattrs are correctly created.
> 
> I also adapted the IMA/EVM tests, which are available here:
> 
> https://github.com/robertosassu/ima-evm-utils/tree/evm-multiple-lsms-nicolas-v1-devel-v6
> 
> Nicolas, if you want to test the new patch locally, build the UML
> kernel with:
Will surely do ! Thanks.
> 
> make ARCH=um -j$(nproc)
> 
> Build ima-evm-utils, and copy linux and certs/signing_key.pem from the
> kernel source directory to the ima-evm-utils directory. Then, run:
> 
> tests/evm_multiple_lsms.test
> 
> It basically runs the UML kernel with different combinations of LSMs
> (some providing an xattr, some not) and compares the HMAC calculated by
> EVM in the kernel with the HMAC calculated by evmctl in user space.
> 
> Roberto
> 

Best regards,

Nicolas Bouchinet

Re: [PATCH v4] evm: Correct inode_init_security hooks behaviors

[Roberto Sassu]

On Fri, 2022-11-18 at 15:20 +0100, Nicolas Bouchinet wrote:
> On Fri, Nov 04, 2022 at 02:43:25PM +0100, Roberto Sassu wrote:
> > On Thu, 2022-11-03 at 16:27 +0100, Roberto Sassu wrote:
> > > On Wed, 2022-10-26 at 16:30 +0200, Nicolas Bouchinet wrote:
> > > > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > > > 
> > > > Fixes a NULL pointer dereference occurring 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.
> > > > 
> > > > The `call_int_hook_xattr` macro has been inlined into the
> > > > `security_inode_init_security` hook in order to check hooks return
> > > > values and skip ones who doesn't init `xattrs`.
> > > > 
> > > > Modify `evm_init_hmac` function to init the EVM hmac using every
> > > > entry of the given xattr array.
> > > > 
> > > > The `MAX_LSM_EVM_XATTR` value is now based on the security modules
> > > > compiled in, 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: roberto.sassu@huaweicloud.com
> > > > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > > > ---
> > > > Changes since v3:
> > > > https://lore.kernel.org/linux-integrity/Y1fu4jofqLHVDprT@archlinux/
> > > > 
> > > > * Fixes compilation error reported by the kernel test robot.
> > > > ---
> > > >  include/linux/lsm_hook_defs.h       |  2 +-
> > > >  include/linux/lsm_hooks.h           |  4 ++--
> > > >  security/integrity/evm/evm.h        |  1 +
> > > >  security/integrity/evm/evm_crypto.c |  9 +++++++--
> > > >  security/integrity/evm/evm_main.c   |  7 ++++---
> > > >  security/security.c                 | 31 ++++++++++++++++++++++-------
> > > >  6 files changed, 39 insertions(+), 15 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.
> > > 
> > > Please also update the description of @name as well (remove allocated).
> > 
> > While you update the patch, I worked on the other patches: reiserfs
> > fixes, if we want still to apply them; expand the call_int_hook() loop
> > also for security_old_inode_init_security() to have consistent behavior
> > across all filesystems.
> 
> Thank's I didn't had time to read the patch, will do it.

Hi Nicolas

in the meantime we went further. As Paul commented that he would prefer
to use the reservation mechanism for xattrs, instead of a static
allocation, I resumed my old patch set, which would include also your
changes.

Will send a new version shortly. Will appreciate your feedback!

Thanks

Roberto

> > The patches are available here:
> > 
> > https://github.com/robertosassu/linux/tree/evm-multiple-lsms-nicolas-v1-devel-v6
> > 
> > Other than Github Actions related patches, there is also TestLSM, which
> > I developed to ensure that xattrs are correctly created.
> > 
> > I also adapted the IMA/EVM tests, which are available here:
> > 
> > https://github.com/robertosassu/ima-evm-utils/tree/evm-multiple-lsms-nicolas-v1-devel-v6
> > 
> > Nicolas, if you want to test the new patch locally, build the UML
> > kernel with:
> Will surely do ! Thanks.
> > make ARCH=um -j$(nproc)
> > 
> > Build ima-evm-utils, and copy linux and certs/signing_key.pem from the
> > kernel source directory to the ima-evm-utils directory. Then, run:
> > 
> > tests/evm_multiple_lsms.test
> > 
> > It basically runs the UML kernel with different combinations of LSMs
> > (some providing an xattr, some not) and compares the HMAC calculated by
> > EVM in the kernel with the HMAC calculated by evmctl in user space.
> > 
> > Roberto
> > 
> 
> Best regards,
> 
> Nicolas Bouchinet