| Message ID | 20240730051625.14349-35-viro@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | [01/39] memcg_write_event_control(): fix a user-triggerable oops | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
On Mon, Jul 29, 2024 at 10:27 PM <viro@kernel.org> wrote: > > From: Al Viro <viro@zeniv.linux.org.uk> > > keep file reference through the entire thing, don't bother with > grabbing struct path reference (except, for now, around the LSM > call and that only until it gets constified) and while we are > at it, don't confuse the hell out of readers by random mix of > path.dentry->d_sb and path.mnt->mnt_sb uses - these two are equal, > so just put one of those into a local variable and use that. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > kernel/bpf/token.c | 69 +++++++++++++++++----------------------------- > 1 file changed, 26 insertions(+), 43 deletions(-) > LGTM overall (modulo // comments, but see below) Acked-by: Andrii Nakryiko <andrii@kernel.org> > diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c > index 9b92cb886d49..15da405d8302 100644 > --- a/kernel/bpf/token.c > +++ b/kernel/bpf/token.c > @@ -116,67 +116,52 @@ int bpf_token_create(union bpf_attr *attr) [...] > - err = security_bpf_token_create(token, attr, &path); > + path_get(&path); // kill it > + err = security_bpf_token_create(token, attr, &path); // constify > + path_put(&path); // kill it > if (err) > goto out_token; > By constify you mean something like below? commit 06a6442ca9cc441805881eea61fd57d7defadaca Author: Andrii Nakryiko <andrii@kernel.org> Date: Tue Aug 6 15:38:12 2024 -0700 security: constify struct path in bpf_token_create() LSM hook There is no reason why struct path pointer shouldn't be const-qualified when being passed into bpf_token_create() LSM hook. Add that const. Suggested-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 855db460e08b..462b55378241 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -431,7 +431,7 @@ LSM_HOOK(int, 0, bpf_prog_load, struct bpf_prog *prog, union bpf_attr *attr, struct bpf_token *token) LSM_HOOK(void, LSM_RET_VOID, bpf_prog_free, struct bpf_prog *prog) LSM_HOOK(int, 0, bpf_token_create, struct bpf_token *token, union bpf_attr *attr, - struct path *path) + const struct path *path) LSM_HOOK(void, LSM_RET_VOID, bpf_token_free, struct bpf_token *token) LSM_HOOK(int, 0, bpf_token_cmd, const struct bpf_token *token, enum bpf_cmd cmd) LSM_HOOK(int, 0, bpf_token_capable, const struct bpf_token *token, int cap) diff --git a/include/linux/security.h b/include/linux/security.h index 1390f1efb4f0..31523a2c71c4 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -2137,7 +2137,7 @@ extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, struct bpf_token *token); extern void security_bpf_prog_free(struct bpf_prog *prog); extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, - struct path *path); + const struct path *path); extern void security_bpf_token_free(struct bpf_token *token); extern int security_bpf_token_cmd(const struct bpf_token *token, enum bpf_cmd cmd); extern int security_bpf_token_capable(const struct bpf_token *token, int cap); @@ -2177,7 +2177,7 @@ static inline void security_bpf_prog_free(struct bpf_prog *prog) { } static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, - struct path *path) + const struct path *path) { return 0; } diff --git a/security/security.c b/security/security.c index 8cee5b6c6e6d..d8d0b67ced25 100644 --- a/security/security.c +++ b/security/security.c @@ -5510,7 +5510,7 @@ int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, * Return: Returns 0 on success, error on failure. */ int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, - struct path *path) + const struct path *path) { return call_int_hook(bpf_token_create, token, attr, path); } diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 55c78c318ccd..0eec141a8f37 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6965,7 +6965,7 @@ static void selinux_bpf_prog_free(struct bpf_prog *prog) } static int selinux_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, - struct path *path) + const struct path *path) { struct bpf_security_struct *bpfsec; [...]
On Tue, Jul 30, 2024 at 01:16:21AM GMT, viro@kernel.org wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > keep file reference through the entire thing, don't bother with > grabbing struct path reference (except, for now, around the LSM > call and that only until it gets constified) and while we are > at it, don't confuse the hell out of readers by random mix of > path.dentry->d_sb and path.mnt->mnt_sb uses - these two are equal, > so just put one of those into a local variable and use that. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- Reviewed-by: Christian Brauner <brauner@kernel.org>
On Tue, Aug 06, 2024 at 03:42:56PM -0700, Andrii Nakryiko wrote:
> By constify you mean something like below?
Yep. Should go through LSM folks, probably, and once it's in
those path_get() and path_put() around the call can go to hell,
along with 'path' itself (we can use file->f_path instead).
On Fri, Aug 9, 2024 at 8:46 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Tue, Aug 06, 2024 at 03:42:56PM -0700, Andrii Nakryiko wrote: > > > By constify you mean something like below? > > Yep. Should go through LSM folks, probably, and once it's in > those path_get() and path_put() around the call can go to hell, > along with 'path' itself (we can use file->f_path instead). Ok, cool. Let's then do that branch you proposed, and I can send everything on top of it, removing path_get()+path_put() you add here.
diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c index 9b92cb886d49..15da405d8302 100644 --- a/kernel/bpf/token.c +++ b/kernel/bpf/token.c @@ -116,67 +116,52 @@ int bpf_token_create(union bpf_attr *attr) struct user_namespace *userns; struct inode *inode; struct file *file; + CLASS(fd, f)(attr->token_create.bpffs_fd); struct path path; - struct fd f; + struct super_block *sb; umode_t mode; int err, fd; - f = fdget(attr->token_create.bpffs_fd); - if (!fd_file(f)) + if (fd_empty(f)) return -EBADF; path = fd_file(f)->f_path; - path_get(&path); - fdput(f); + sb = path.dentry->d_sb; - if (path.dentry != path.mnt->mnt_sb->s_root) { - err = -EINVAL; - goto out_path; - } - if (path.mnt->mnt_sb->s_op != &bpf_super_ops) { - err = -EINVAL; - goto out_path; - } + if (path.dentry != sb->s_root) + return -EINVAL; + if (sb->s_op != &bpf_super_ops) + return -EINVAL; err = path_permission(&path, MAY_ACCESS); if (err) - goto out_path; + return err; - userns = path.dentry->d_sb->s_user_ns; + userns = sb->s_user_ns; /* * Enforce that creators of BPF tokens are in the same user * namespace as the BPF FS instance. This makes reasoning about * permissions a lot easier and we can always relax this later. */ - if (current_user_ns() != userns) { - err = -EPERM; - goto out_path; - } - if (!ns_capable(userns, CAP_BPF)) { - err = -EPERM; - goto out_path; - } + if (current_user_ns() != userns) + return -EPERM; + if (!ns_capable(userns, CAP_BPF)) + return -EPERM; /* Creating BPF token in init_user_ns doesn't make much sense. */ - if (current_user_ns() == &init_user_ns) { - err = -EOPNOTSUPP; - goto out_path; - } + if (current_user_ns() == &init_user_ns) + return -EOPNOTSUPP; - mnt_opts = path.dentry->d_sb->s_fs_info; + mnt_opts = sb->s_fs_info; if (mnt_opts->delegate_cmds == 0 && mnt_opts->delegate_maps == 0 && mnt_opts->delegate_progs == 0 && - mnt_opts->delegate_attachs == 0) { - err = -ENOENT; /* no BPF token delegation is set up */ - goto out_path; - } + mnt_opts->delegate_attachs == 0) + return -ENOENT; /* no BPF token delegation is set up */ mode = S_IFREG | ((S_IRUSR | S_IWUSR) & ~current_umask()); - inode = bpf_get_inode(path.mnt->mnt_sb, NULL, mode); - if (IS_ERR(inode)) { - err = PTR_ERR(inode); - goto out_path; - } + inode = bpf_get_inode(sb, NULL, mode); + if (IS_ERR(inode)) + return PTR_ERR(inode); inode->i_op = &bpf_token_iops; inode->i_fop = &bpf_token_fops; @@ -185,8 +170,7 @@ int bpf_token_create(union bpf_attr *attr) file = alloc_file_pseudo(inode, path.mnt, BPF_TOKEN_INODE_NAME, O_RDWR, &bpf_token_fops); if (IS_ERR(file)) { iput(inode); - err = PTR_ERR(file); - goto out_path; + return PTR_ERR(file); } token = kzalloc(sizeof(*token), GFP_USER); @@ -205,7 +189,9 @@ int bpf_token_create(union bpf_attr *attr) token->allowed_progs = mnt_opts->delegate_progs; token->allowed_attachs = mnt_opts->delegate_attachs; - err = security_bpf_token_create(token, attr, &path); + path_get(&path); // kill it + err = security_bpf_token_create(token, attr, &path); // constify + path_put(&path); // kill it if (err) goto out_token; @@ -218,15 +204,12 @@ int bpf_token_create(union bpf_attr *attr) file->private_data = token; fd_install(fd, file); - path_put(&path); return fd; out_token: bpf_token_free(token); out_file: fput(file); -out_path: - path_put(&path); return err; }