[PATCH v4] evm: Correct inode_init_security hooks behaviors

[Paul Moore]

On Wed, Oct 26, 2022 at 10:30 AM Nicolas Bouchinet
<nicolas.bouchinet at clip-os.org> wrote:
>
> From: Nicolas Bouchinet <nicolas.bouchinet at 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 at huaweicloud.com
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet at 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