[PATCH] evm: Correct inode_init_security hooks behaviors

[Paul Moore]

On Thu, Oct 20, 2022 at 9:55 AM Nicolas Bouchinet
<nicolas.bouchinet at clip-os.org> wrote:
>
> From: Nicolas Bouchinet <nicolas.bouchinet at 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 at vger.kernel.org
> Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet at 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