evm: Correct inode_init_security hooks behaviors
Subsystems (5)
PATCH v3 1/1 evm: Correct inode_init_security hooks behaviors
From: Nicolas Bouchinet <[email protected]>
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).
---
Changes since v2:
https://lore.kernel.org/linux-integrity/Y1K3bf+dtNnVe7DG@archlinux/
* The evm_init_hmacs has been merged with evm_init_hmac (as suggested by Mimi Zohar).
* The commit message has been updated to match the patch changes.
--- include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 4 ++-- security/integrity/evm/evm_crypto.c | 11 ++++++++-- security/integrity/evm/evm_main.c | 7 ++++--- security/security.c | 31 ++++++++++++++++++++++------- 5 files changed, 40 insertions(+), 15 deletions(-)
include/linux/lsm_hook_defs.h +1 -1
@@ -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,
include/linux/lsm_hooks.h +2 -2
@@ -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.
security/integrity/evm/evm_crypto.c +9 -2
@@ -385,7 +385,9 @@ 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_protected_xattr (const char *req_xattr_name); + +int evm_init_hmac (struct inode *inode,const struct xattr *lsm_xattrs,char *hmac_val) {struct shash_desc *desc;
@@ -396,7 +398,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 ;
security/integrity/evm/evm_main.c +4 -3
@@ -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 );
security/security.c +24 -7
@@ -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);
Signature · 1 line
2.38.1
kernel test robot
build botThank you for the patch! Perhaps something to improve:
Hi Nicolas,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on zohar-integrity/next-integrity] [also build test WARNING on linus/master v6.1-rc2 next-20221025] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nicolas-Bouchinet/evm-Correct-inode_init_security-hooks-behaviors/20221025-221337 base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity patch link: https://lore.kernel.org/r/Y1fu4jofqLHVDprT%40archlinux patch subject: [PATCH v3] evm: Correct inode_init_security hooks behaviors config: m68k-allyesconfig (attached as .config) compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/acbbdcb851ed7cd1894efad182f0325db04b9e5a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nicolas-Bouchinet/evm-Correct-inode_init_security-hooks-behaviors/20221025-221337 git checkout acbbdcb851ed7cd1894efad182f0325db04b9e5a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash security/integrity/
If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> security/integrity/evm/evm_main.c:310:5: warning: no previous prototype for 'evm_protected_xattr' [-Wmissing-prototypes]
310 | int evm_protected_xattr(const char *req_xattr_name)
| ^~~~~~~~~~~~~~~~~~~
vim +/evm_protected_xattr +310 security/integrity/evm/evm_main.c
309
> 310 int evm_protected_xattr(const char *req_xattr_name)
311 {
312 return evm_protected_xattr_common(req_xattr_name, false);
313 }
314
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
kernel test robot
build botThank you for the patch! Perhaps something to improve:
Hi Nicolas,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on zohar-integrity/next-integrity] [also build test WARNING on linus/master v6.1-rc2 next-20221025] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nicolas-Bouchinet/evm-Correct-inode_init_security-hooks-behaviors/20221025-221337 base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity patch link: https://lore.kernel.org/r/Y1fu4jofqLHVDprT%40archlinux patch subject: [PATCH v3] evm: Correct inode_init_security hooks behaviors config: hexagon-randconfig-r002-20221023 (attached as .config) compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/acbbdcb851ed7cd1894efad182f0325db04b9e5a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Nicolas-Bouchinet/evm-Correct-inode_init_security-hooks-behaviors/20221025-221337 git checkout acbbdcb851ed7cd1894efad182f0325db04b9e5a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash security/integrity/evm/
If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> security/integrity/evm/evm_main.c:310:5: warning: no previous prototype for function 'evm_protected_xattr' [-Wmissing-prototypes]
int evm_protected_xattr(const char *req_xattr_name)
^ security/integrity/evm/evm_main.c:310:1: note: declare 'static' if the function is not intended to be used outside of this translation unit int evm_protected_xattr(const char *req_xattr_name)
^
static
1 warning generated.
vim +/evm_protected_xattr +310 security/integrity/evm/evm_main.c
309
> 310 int evm_protected_xattr(const char *req_xattr_name)
311 {
312 return evm_protected_xattr_common(req_xattr_name, false);
313 }
314
Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp