lkml.org 
[lkml]   [2019]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH bpf-next v9 05/10] bpf,landlock: Add a new map type: inode
On Tue, Jun 25, 2019 at 11:52:34PM +0200, Mickaël Salaün wrote:
> +/* must call iput(inode) after this call */
> +static struct inode *inode_from_fd(int ufd, bool check_access)
> +{
> + struct inode *ret;
> + struct fd f;
> + int deny;
> +
> + f = fdget(ufd);
> + if (unlikely(!f.file || !file_inode(f.file))) {
> + ret = ERR_PTR(-EBADF);
> + goto put_fd;
> + }

Just when does one get a NULL file_inode()? The reason I'm asking is
that arseloads of code would break if one managed to create such
a beast...

Incidentally, that should be return ERR_PTR(-EBADF); fdput() is wrong there.

> + }
> + /* check if the FD is tied to a mount point */
> + /* TODO: add this check when called from an eBPF program too */
> + if (unlikely(!f.file->f_path.mnt

Again, the same question - when the hell can that happen? If you are
sitting on an exploitable roothole, do share it...

|| f.file->f_path.mnt->mnt_flags &
> + MNT_INTERNAL)) {
> + ret = ERR_PTR(-EINVAL);
> + goto put_fd;

What does it have to do with mountpoints, anyway?

> +/* called from syscall */
> +static int sys_inode_map_delete_elem(struct bpf_map *map, struct inode *key)
> +{
> + struct inode_array *array = container_of(map, struct inode_array, map);
> + struct inode *inode;
> + int i;
> +
> + WARN_ON_ONCE(!rcu_read_lock_held());
> + for (i = 0; i < array->map.max_entries; i++) {
> + if (array->elems[i].inode == key) {
> + inode = xchg(&array->elems[i].inode, NULL);
> + array->nb_entries--;

Umm... Is that intended to be atomic in any sense?

> + iput(inode);
> + return 0;
> + }
> + }
> + return -ENOENT;
> +}
> +
> +/* called from syscall */
> +int bpf_inode_map_delete_elem(struct bpf_map *map, int *key)
> +{
> + struct inode *inode;
> + int err;
> +
> + inode = inode_from_fd(*key, false);
> + if (IS_ERR(inode))
> + return PTR_ERR(inode);
> + err = sys_inode_map_delete_elem(map, inode);
> + iput(inode);
> + return err;
> +}

Wait a sec... So we have those beasties that can have long-term
references to arbitrary inodes stuck in them? What will happen
if you get umount(2) called while such a thing exists?

\
 
 \ /
  Last update: 2019-06-26 00:52    [W:0.123 / U:46.820 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site