| Message ID | 20241217063821.482857-5-song@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Enable writing xattr from BPF programs | expand |
On Mon, Dec 16, 2024 at 10:38 PM Song Liu <song@kernel.org> 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. ... > + * > + * 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. > + */ the inode locking rules might change, so let's hide this implementation detail from the bpf progs by making kfunc polymorphic. To struct bpf_prog_aux add: bool use_locked_kfunc:1; and set it in bpf_check_attach_target() if it's attaching to one of d_inode_locked_hooks Then in fixup_kfunc_call() call some helper that if (prog->aux->use_locked_kfunc && insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr]) insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked]; The progs will be simpler and will suffer less churn when the kernel side changes.
Hi Alexei, Thanks for the review! > On Dec 17, 2024, at 8:50 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Dec 16, 2024 at 10:38 PM Song Liu <song@kernel.org> 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. > > ... > >> + * >> + * 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. >> + */ > > the inode locking rules might change, so let's hide this > implementation detail from the bpf progs by making kfunc polymorphic. > > To struct bpf_prog_aux add: > bool use_locked_kfunc:1; > and set it in bpf_check_attach_target() if it's attaching > to one of d_inode_locked_hooks > > Then in fixup_kfunc_call() call some helper that > if (prog->aux->use_locked_kfunc && > insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr]) > insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked]; > > The progs will be simpler and will suffer less churn > when the kernel side changes. I was thinking about something in similar direction. If we do this, shall we somehow hide the _locked version of the kfuncs, so that the user cannot use it? If so, what's the best way to do it? Thanks, Song
On Tue, 17 Dec 2024 at 19:25, Song Liu <songliubraving@meta.com> wrote: > > Hi Alexei, > > Thanks for the review! > > > On Dec 17, 2024, at 8:50 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > On Mon, Dec 16, 2024 at 10:38 PM Song Liu <song@kernel.org> 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. > > > > ... > > > >> + * > >> + * 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. > >> + */ > > > > the inode locking rules might change, so let's hide this > > implementation detail from the bpf progs by making kfunc polymorphic. > > > > To struct bpf_prog_aux add: > > bool use_locked_kfunc:1; > > and set it in bpf_check_attach_target() if it's attaching > > to one of d_inode_locked_hooks > > > > Then in fixup_kfunc_call() call some helper that > > if (prog->aux->use_locked_kfunc && > > insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr]) > > insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked]; > > > > The progs will be simpler and will suffer less churn > > when the kernel side changes. > > I was thinking about something in similar direction. > > If we do this, shall we somehow hide the _locked version of the > kfuncs, so that the user cannot use it? If so, what's the best > way to do it? Just don't add BTF_ID_FLAGS entries for them. You'd also need to make an extra call to add_kfunc_call to add its details before you can do the fixup. That allows find_kfunc_desc to work. I did something similar in earlier versions of resilient locks. In add_kfunc_call's end (instead of directly returning): func_id = get_shadow_kfunc_id(func_id, offset); if (!func_id) return err; return add_kfunc_call(env, func_id, offset); Then check in fixup_kfunc_call to find shadow kfunc id and substitute imm. Can use some other naming instead of "shadow". Probably need to take a prog pointer to make a decision to find the underlying kfunc id in your case. > > Thanks, > Song >
> On Dec 17, 2024, at 10:32 AM, Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Tue, 17 Dec 2024 at 19:25, Song Liu <songliubraving@meta.com> wrote: >> >> Hi Alexei, >> >> Thanks for the review! >> >>> On Dec 17, 2024, at 8:50 AM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: >>> >>> On Mon, Dec 16, 2024 at 10:38 PM Song Liu <song@kernel.org> 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. >>> >>> ... >>> >>>> + * >>>> + * 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. >>>> + */ >>> >>> the inode locking rules might change, so let's hide this >>> implementation detail from the bpf progs by making kfunc polymorphic. >>> >>> To struct bpf_prog_aux add: >>> bool use_locked_kfunc:1; >>> and set it in bpf_check_attach_target() if it's attaching >>> to one of d_inode_locked_hooks >>> >>> Then in fixup_kfunc_call() call some helper that >>> if (prog->aux->use_locked_kfunc && >>> insn->imm == special_kfunc_list[KF_bpf_remove_dentry_xattr]) >>> insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked]; >>> >>> The progs will be simpler and will suffer less churn >>> when the kernel side changes. >> >> I was thinking about something in similar direction. >> >> If we do this, shall we somehow hide the _locked version of the >> kfuncs, so that the user cannot use it? If so, what's the best >> way to do it? > > Just don't add BTF_ID_FLAGS entries for them. > You'd also need to make an extra call to add_kfunc_call to add its > details before you can do the fixup. > That allows find_kfunc_desc to work. > I did something similar in earlier versions of resilient locks. > In add_kfunc_call's end (instead of directly returning): > func_id = get_shadow_kfunc_id(func_id, offset); > if (!func_id) > return err; > return add_kfunc_call(env, func_id, offset); > > Then check in fixup_kfunc_call to find shadow kfunc id and substitute imm. > Can use some other naming instead of "shadow". > Probably need to take a prog pointer to make a decision to find the > underlying kfunc id in your case. Thanks for the hints! They helped a lot. I ended up doing this with a slightly different logic, which I think is cleaner. I will send v5 shortly. Song
diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c index 8a65184c8c2c..3261324e956c 100644 --- a/fs/bpf_fs_kfuncs.c +++ b/fs/bpf_fs_kfuncs.c @@ -6,6 +6,7 @@ #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 +162,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(). + */ + } +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); +} + +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) @@ -186,9 +345,91 @@ static const struct btf_kfunc_id_set bpf_fs_kfunc_set = { .filter = bpf_fs_kfuncs_filter, }; +/* 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_KFUNCS_START(bpf_write_xattr_set_ids) +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_write_xattr_set_ids) + +static int bpf_write_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id) +{ + if (!btf_id_set8_contains(&bpf_write_xattr_set_ids, kfunc_id)) + return 0; + if (prog->type != BPF_PROG_TYPE_LSM) + return -EACCES; + + if (bpf_lsm_has_d_inode_locked(prog)) + return -EINVAL; + return 0; +} + +static const struct btf_kfunc_id_set bpf_write_xattr_set = { + .owner = THIS_MODULE, + .set = &bpf_write_xattr_set_ids, + .filter = bpf_write_xattr_filter, +}; + +BTF_KFUNCS_START(bpf_write_xattr_locked_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_write_xattr_locked_set_ids) + +static int bpf_write_xattr_locked_filter(const struct bpf_prog *prog, u32 kfunc_id) +{ + if (!btf_id_set8_contains(&bpf_write_xattr_locked_set_ids, kfunc_id)) + return 0; + if (prog->type != BPF_PROG_TYPE_LSM) + return -EACCES; + + if (!bpf_lsm_has_d_inode_locked(prog)) + return -EINVAL; + return 0; +} + +static const struct btf_kfunc_id_set bpf_write_xattr_locked_set = { + .owner = THIS_MODULE, + .set = &bpf_write_xattr_locked_set_ids, + .filter = bpf_write_xattr_locked_filter, +}; + static int __init bpf_fs_kfuncs_init(void) { - return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set); + int ret; + + ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set); + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_write_xattr_set); + ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_write_xattr_locked_set); + return ret; } late_initcall(bpf_fs_kfuncs_init);
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. Signed-off-by: Song Liu <song@kernel.org> --- fs/bpf_fs_kfuncs.c | 243 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 242 insertions(+), 1 deletion(-)