netdev - Re: [PATCH v5 bpf-next 03/13] bpf: introduce BPF token object

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 27 Sep 2023 09:03:31 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	keescook@...omium.org, lennart@...ttering.net, kernel-team@...a.com, 
	sargun@...gun.me
Subject: Re: [PATCH v5 bpf-next 03/13] bpf: introduce BPF token object

On Wed, Sep 27, 2023 at 2:52 AM Christian Brauner <brauner@...nel.org> wrote:
>
> > > > +#define BPF_TOKEN_INODE_NAME "bpf-token"
> > > > +
> > > > +/* Alloc anon_inode and FD for prepared token.
> > > > + * Returns fd >= 0 on success; negative error, otherwise.
> > > > + */
> > > > +int bpf_token_new_fd(struct bpf_token *token)
> > > > +{
> > > > +     return anon_inode_getfd(BPF_TOKEN_INODE_NAME, &bpf_token_fops, token, O_CLOEXEC);
> > >
> > > It's unnecessary to use the anonymous inode infrastructure for bpf
> > > tokens. It adds even more moving parts and makes reasoning about it even
> > > harder. Just keep it all in bpffs. IIRC, something like the following
> > > (broken, non-compiling draft) should work:
> > >
> > > /* bpf_token_file - get an unlinked file living in bpffs */
> > > struct file *bpf_token_file(...)
> > > {
> > >         inode = bpf_get_inode(bpffs_mnt->mnt_sb, dir, mode);
> > >         inode->i_op = &bpf_token_iop;
> > >         inode->i_fop = &bpf_token_fops;
> > >
> > >         // some other stuff you might want or need
> > >
> > >         res = alloc_file_pseudo(inode, bpffs_mnt, "bpf-token", O_RDWR, &bpf_token_fops);
> > > }
> > >
> > > Now set your private data that you might need, reserve an fd, install
> > > the file into the fdtable and return the fd. You should have an unlinked
> > > bpffs file that serves as your bpf token.
> >
> > Just to make sure I understand. You are saying that instead of having
> > `struct bpf_token *` and passing that into internal APIs
> > (bpf_token_capable() and bpf_token_allow_xxx()), I should just pass
> > around `struct super_block *` representing BPF FS instance? Or `struct
> > bpf_mount_opts *` maybe? Or 'struct vfsmount *'? (Any preferences
> > here?). Is that right?
>
> No, that's not what I meant.
>
> So, what you're doing right now to create a bpf token file descriptor is:
>
> return anon_inode_getfd(BPF_TOKEN_INODE_NAME, &bpf_token_fops, token, O_CLOEXEC);
>
> which is using the anonymous inode infrastructure. That is an entirely
> different filesystems (glossing over details) that is best leveraged for
> stuff like kvm fds and other stuff that doesn't need or have its own
> filesytem implementation.
>
> But you do have your own filesystem implementation so why abuse another
> one to create bpf token fds when they can just be created directly from
> the bpffs instance.
>
> IOW, everything stays the same apart from the fact that bpf token fds
> are actually file descriptors referring to a detached bpffs file instead
> of an anonymous inode file. IOW, bpf tokens are actual bpffs objects
> tied to a bpffs instance.

Ah, ok, this is a much smaller change than what I was about to make.
I'm glad I asked and thanks for elaborating! I'll use
alloc_file_pseudo() using bpffs mount in the next revision.

>
> **BROKEN BROKEN BROKEN AND UGLY**
>
> int bpf_token_create(union bpf_attr *attr)
> {
>         struct inode *inode;
>         struct path path;
>         struct bpf_mount_opts *mnt_opts;
>         struct bpf_token *token;
>         struct fd fd;
>         int fd, ret;
>         struct file *file;
>
>         fd = fdget(attr->token_create.bpffs_path_fd);
>         if (!fd.file)
>                 goto cleanup;
>
>         if (fd.file->f_path->dentry != fd.file->f_path->dentry->d_sb->s_root)
>                 goto cleanup;
>
>         inode = bpf_get_inode(fd.file->f_path->mnt->mnt_sb, NULL, 1234123412341234);
>         if (!inode)
>                 goto cleanup;
>
>         fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
>         if (fd < 0)
>                 goto cleanup;
>
>         clear_nlink(inode); /* make sure it is unlinked */
>
>         file = alloc_file_pseudo(inode, fd.file->f_path->mnt, "bpf-token", O_RDWR, &&bpf_token_fops);
>         if (IS_ERR(file))
>                 goto cleanup;
>
>         token = bpf_token_alloc();
>         if (!token)
>                 goto cleanup;
>
>         /* remember bpffs owning userns for future ns_capable() checks */
>         token->userns = get_user_ns(path.dentry->d_sb->s_user_ns);
>
>         mnt_opts = path.dentry->d_sb->s_fs_info;
>         token->allowed_cmds = mnt_opts->delegate_cmds;
>         token->allowed_maps = mnt_opts->delegate_maps;
>         token->allowed_progs = mnt_opts->delegate_progs;
>         token->allowed_attachs = mnt_opts->delegate_attachs;
>
>         file->private_data = token;
>         fd_install(fd, file);
>         return fd;
>
> cleanup:
>         // cleanup stuff here
>         return -SOME_ERROR;
> }