On Fri, Oct 21, 2022 at 9:57 AM Roberto Sassu
<roberto.sassu@xxxxxxxxxxxxxxx> wrote:
>
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>
> BPF LSM allows security modules to directly attach to the security hooks,
> with the potential of not meeting the kernel expectation.
>
> This is the case for the inode_init_security hook, for which the kernel
> expects that name and value are set if the hook implementation returns
> zero.
>
> Consequently, not meeting the kernel expectation can cause the kernel to
> crash. One example is evm_protected_xattr_common() which expects the
> req_xattr_name parameter to be always not NULL.
Sounds like a bug in evm_protected_xattr_common.
> Introduce a level of indirection in BPF LSM, for the inode_init_security
> hook, to check the validity of the name and value set by security modules.
Doesn't make sense.
You probably meant security_old_inode_init_security,
because the hook without _old_ doesn't have such args:
int security_inode_init_security(struct inode *inode, struct inode *dir,
const struct qstr *qstr,
initxattrs initxattrs, void *fs_data);
> Encapsulate bpf_lsm_inode_init_security(), the existing attachment point,
> with bpf_inode_init_security(), the new function. After the attachment
> point is called, return -EOPNOTSUPP if the xattr name is not set, -ENOMEM
> if the xattr value is not set.
>
> As the name still cannot be set, rely on future patches to the eBPF
> verifier or introducing new kfuncs/helpers to ensure its correctness.
>
> Finally, as proposed by Nicolas, update the LSM hook documentation for the
> inode_init_security hook, to reflect the current behavior (only the xattr
> value is allocated).
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 520b7aa00d8cd ("bpf: lsm: Initialize the BPF LSM hooks")
> Reported-by: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
> include/linux/lsm_hooks.h | 4 ++--
> security/bpf/hooks.c | 25 +++++++++++++++++++++++++
> 2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e..f44d45f4737f 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.
must be an obsolete comment.
> * 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/bpf/hooks.c b/security/bpf/hooks.c
> index e5971fa74fd7..492c07ba6722 100644
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
> @@ -6,11 +6,36 @@
> #include <linux/lsm_hooks.h>
> #include <linux/bpf_lsm.h>
>
> +static int bpf_inode_init_security(struct inode *inode, struct inode *dir,
> + const struct qstr *qstr, const char **name,
> + void **value, size_t *len)
> +{
> + int ret;
> +
> + ret = bpf_lsm_inode_init_security(inode, dir, qstr, name, value, len);
> + if (ret)
> + return ret;
> +
> + /*
> + * As the name cannot be set by the eBPF programs directly, eBPF will
> + * be responsible for its correctness through the verifier or
> + * appropriate kfuncs/helpers.
> + */
> + if (name && !*name)
> + return -EOPNOTSUPP;
bpf cannot write into such pointers.
It won't be able to use kfuncs to kmalloc and write into them either.
None of it makes sense to me.
> +
> + if (value && !*value)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
> #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),
> #include <linux/lsm_hook_defs.h>
> #undef LSM_HOOK
> + LSM_HOOK_INIT(inode_init_security, bpf_inode_init_security),
> LSM_HOOK_INIT(inode_free_security, bpf_inode_storage_free),
> LSM_HOOK_INIT(task_free, bpf_task_storage_free),
> };
> --
> 2.25.1
>
