On Thu, Jan 09, 2025 at 05:13:41PM -0800, Song Liu wrote:
> Add the following kfuncs to set and remove xattrs from BPF programs:
>
> bpf_set_dentry_xattr
> bpf_remove_dentry_xattr
> bpf_set_dentry_xattr_locked
> bpf_remove_dentry_xattr_locked
>
> The _locked version of these kfuncs are called from hooks where
> dentry->d_inode is already locked. Instead of requiring the user
> to know which version of the kfuncs to use, the verifier will pick
> the proper kfunc based on the calling hook.
>
> Signed-off-by: Song Liu <song@xxxxxxxxxx>
> ---
> fs/bpf_fs_kfuncs.c | 227 +++++++++++++++++++++++++++++++++++++++-
> include/linux/bpf_lsm.h | 2 +
> 2 files changed, 227 insertions(+), 2 deletions(-)
>
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 8a65184c8c2c..f68c83bfb93f 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
> @@ -2,10 +2,12 @@
> /* Copyright (c) 2024 Google LLC. */
>
> #include <linux/bpf.h>
> +#include <linux/bpf_lsm.h>
> #include <linux/btf.h>
> #include <linux/btf_ids.h>
> #include <linux/dcache.h>
> #include <linux/fs.h>
> +#include <linux/fsnotify.h>
> #include <linux/file.h>
> #include <linux/mm.h>
> #include <linux/xattr.h>
> @@ -161,6 +163,164 @@ __bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
> return bpf_get_dentry_xattr(dentry, name__str, value_p);
> }
>
> +static int bpf_xattr_write_permission(const char *name, struct inode *inode)
> +{
> + if (WARN_ON(!inode))
> + return -EINVAL;
> +
> + /* Only allow setting and removing security.bpf. xattrs */
> + if (!match_security_bpf_prefix(name))
> + return -EPERM;
> +
> + return inode_permission(&nop_mnt_idmap, inode, MAY_WRITE);
> +}
> +
> +static int __bpf_set_dentry_xattr(struct dentry *dentry, const char *name,
> + const struct bpf_dynptr *value_p, int flags, bool lock_inode)
> +{
> + struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
> + struct inode *inode = d_inode(dentry);
> + const void *value;
> + u32 value_len;
> + int ret;
> +
> + value_len = __bpf_dynptr_size(value_ptr);
> + value = __bpf_dynptr_data(value_ptr, value_len);
> + if (!value)
> + return -EINVAL;
> +
> + if (lock_inode)
> + inode_lock(inode);
> +
> + ret = bpf_xattr_write_permission(name, inode);
> + if (ret)
> + goto out;
> +
> + ret = __vfs_setxattr(&nop_mnt_idmap, dentry, inode, name,
> + value, value_len, flags);
> + if (!ret) {
> + fsnotify_xattr(dentry);
> +
> + /* This xattr is set by BPF LSM, so we do not call
> + * security_inode_post_setxattr. This is the same as
> + * security_inode_setsecurity().
> + */
If you did you would risk deadlocks as you could end up calling yourself
again afaict.
> + }
> +out:
> + if (lock_inode)
> + inode_unlock(inode);
> + return ret;
> +}
> +
> +/**
> + * bpf_set_dentry_xattr - set a xattr of a dentry
> + * @dentry: dentry to get xattr from
> + * @name__str: name of the xattr
> + * @value_p: xattr value
> + * @flags: flags to pass into filesystem operations
> + *
> + * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
> + *
> + * For security reasons, only *name__str* with prefix "security.bpf."
> + * is allowed.
> + *
> + * The caller has not locked dentry->d_inode.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +__bpf_kfunc int bpf_set_dentry_xattr(struct dentry *dentry, const char *name__str,
> + const struct bpf_dynptr *value_p, int flags)
> +{
> + return __bpf_set_dentry_xattr(dentry, name__str, value_p, flags, true);
> +}
> +
> +/**
> + * bpf_set_dentry_xattr_locked - set a xattr of a dentry
> + * @dentry: dentry to get xattr from
> + * @name__str: name of the xattr
> + * @value_p: xattr value
> + * @flags: flags to pass into filesystem operations
> + *
> + * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
> + *
> + * For security reasons, only *name__str* with prefix "security.bpf."
> + * is allowed.
> + *
> + * The caller already locked dentry->d_inode.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +__bpf_kfunc int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
> + const struct bpf_dynptr *value_p, int flags)
> +{
> + return __bpf_set_dentry_xattr(dentry, name__str, value_p, flags, false);
That boolean argument is not needed if you pull
value_len = __bpf_dynptr_size(value_ptr);
value = __bpf_dynptr_data(value_ptr, value_len);
if (!value)
return -EINVAL;
into the two functions and then put:
inode_lock()
bpf_set_dentry_xattr_unlocked();
inode_unlock()
for the locked variant. Similar comment applied to the remove functions.
> +}
> +
> +static int __bpf_remove_dentry_xattr(struct dentry *dentry, const char *name__str,
> + bool lock_inode)
> +{
> + struct inode *inode = d_inode(dentry);
> + int ret;
> +
> + if (lock_inode)
> + inode_lock(inode);
> +
> + ret = bpf_xattr_write_permission(name__str, inode);
> + if (ret)
> + goto out;
> +
> + ret = __vfs_removexattr(&nop_mnt_idmap, dentry, name__str);
> + if (!ret) {
> + fsnotify_xattr(dentry);
> +
> + /* This xattr is removed by BPF LSM, so we do not call
> + * security_inode_post_removexattr.
> + */
> + }
> +out:
> + if (lock_inode)
> + inode_unlock(inode);
> + return ret;
> +}
> +
> +/**
> + * bpf_remove_dentry_xattr - remove a xattr of a dentry
> + * @dentry: dentry to get xattr from
> + * @name__str: name of the xattr
> + *
> + * Rmove xattr *name__str* of *dentry*.
> + *
> + * For security reasons, only *name__str* with prefix "security.bpf."
> + * is allowed.
> + *
> + * The caller has not locked dentry->d_inode.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +__bpf_kfunc int bpf_remove_dentry_xattr(struct dentry *dentry, const char *name__str)
> +{
> + return __bpf_remove_dentry_xattr(dentry, name__str, true);
> +}
> +
> +/**
> + * bpf_remove_dentry_xattr_locked - remove a xattr of a dentry
> + * @dentry: dentry to get xattr from
> + * @name__str: name of the xattr
> + *
> + * Rmove xattr *name__str* of *dentry*.
> + *
> + * For security reasons, only *name__str* with prefix "security.bpf."
> + * is allowed.
> + *
> + * The caller already locked dentry->d_inode.
> + *
> + * Return: 0 on success, a negative value on error.
> + */
> +__bpf_kfunc int bpf_remove_dentry_xattr_locked(struct dentry *dentry, const char *name__str)
> +{
> + return __bpf_remove_dentry_xattr(dentry, name__str, false);
> +}
> +
> __bpf_kfunc_end_defs();
>
> BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
> @@ -170,20 +330,83 @@ BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE)
> BTF_ID_FLAGS(func, bpf_path_d_path, KF_TRUSTED_ARGS)
> BTF_ID_FLAGS(func, bpf_get_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
> BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
> BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
>
> +BTF_HIDDEN_KFUNCS_START(bpf_fs_kfunc_hidden_set_ids)
> +BTF_ID_FLAGS(func, bpf_set_dentry_xattr_locked, KF_SLEEPABLE | KF_TRUSTED_ARGS)
> +BTF_ID_FLAGS(func, bpf_remove_dentry_xattr_locked, KF_SLEEPABLE | KF_TRUSTED_ARGS)
> +BTF_KFUNCS_END(bpf_fs_kfunc_hidden_set_ids)
> +
> static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
> {
> - if (!btf_id_set8_contains(&bpf_fs_kfunc_set_ids, kfunc_id) ||
> - prog->type == BPF_PROG_TYPE_LSM)
> + if (!btf_id_set8_contains(&bpf_fs_kfunc_set_ids, kfunc_id) &&
> + !btf_id_set8_contains(&bpf_fs_kfunc_hidden_set_ids, kfunc_id))
> + return 0;
> + if (prog->type == BPF_PROG_TYPE_LSM)
> return 0;
> return -EACCES;
> }
>
> +/* bpf_[set|remove]_dentry_xattr.* hooks have KF_TRUSTED_ARGS and
> + * KF_SLEEPABLE, so they are only available to sleepable hooks with
> + * dentry arguments.
> + *
> + * Setting and removing xattr requires exclusive lock on dentry->d_inode.
> + * Some hooks already locked d_inode, while some hooks have not locked
> + * d_inode. Therefore, we need different kfuncs for different hooks.
> + * Specifically, hooks in the following list (d_inode_locked_hooks)
> + * should call bpf_[set|remove]_dentry_xattr_locked; while other hooks
> + * should call bpf_[set|remove]_dentry_xattr.
> + */
> +BTF_SET_START(d_inode_locked_hooks)
> +BTF_ID(func, bpf_lsm_inode_post_removexattr)
> +BTF_ID(func, bpf_lsm_inode_post_setattr)
> +BTF_ID(func, bpf_lsm_inode_post_setxattr)
> +BTF_ID(func, bpf_lsm_inode_removexattr)
> +BTF_ID(func, bpf_lsm_inode_rmdir)
> +BTF_ID(func, bpf_lsm_inode_setattr)
> +BTF_ID(func, bpf_lsm_inode_setxattr)
> +BTF_ID(func, bpf_lsm_inode_unlink)
> +#ifdef CONFIG_SECURITY_PATH
> +BTF_ID(func, bpf_lsm_path_unlink)
> +BTF_ID(func, bpf_lsm_path_rmdir)
> +#endif /* CONFIG_SECURITY_PATH */
> +BTF_SET_END(d_inode_locked_hooks)
> +
> +static bool bpf_lsm_has_d_inode_locked(const struct bpf_prog *prog)
> +{
> + return btf_id_set_contains(&d_inode_locked_hooks, prog->aux->attach_btf_id);
> +}
> +
> +BTF_ID_LIST(not_locked_fs_kfuncs)
> +BTF_ID(func, bpf_set_dentry_xattr)
> +BTF_ID(func, bpf_remove_dentry_xattr)
> +
> +BTF_ID_LIST(locked_fs_kfuncs)
> +BTF_ID(func, bpf_set_dentry_xattr_locked)
> +BTF_ID(func, bpf_remove_dentry_xattr_locked)
> +
> +static u32 bpf_fs_kfunc_remap(const struct bpf_prog *prog, u32 kfunc_id)
> +{
> + if (!bpf_lsm_has_d_inode_locked(prog))
> + return 0;
> +
> + if (kfunc_id == not_locked_fs_kfuncs[0])
> + return locked_fs_kfuncs[0];
> + if (kfunc_id == not_locked_fs_kfuncs[1])
> + return locked_fs_kfuncs[1];
> +
> + return 0;
> +}
> +
> static const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
> .owner = THIS_MODULE,
> .set = &bpf_fs_kfunc_set_ids,
> + .hidden_set = &bpf_fs_kfunc_hidden_set_ids,
> .filter = bpf_fs_kfuncs_filter,
> + .remap = bpf_fs_kfunc_remap,
> };
>
> static int __init bpf_fs_kfuncs_init(void)
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index aefcd6564251..f4ab0dc1df69 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -48,6 +48,7 @@ void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, bpf_func_t *bpf_func)
>
> int bpf_lsm_get_retval_range(const struct bpf_prog *prog,
> struct bpf_retval_range *range);
> +
> #else /* !CONFIG_BPF_LSM */
>
> static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
> @@ -86,6 +87,7 @@ static inline int bpf_lsm_get_retval_range(const struct bpf_prog *prog,
> {
> return -EOPNOTSUPP;
> }
> +
> #endif /* CONFIG_BPF_LSM */
>
> #endif /* _LINUX_BPF_LSM_H */
> --
> 2.43.5
>
