On Tue, Jun 25, 2019 at 11:52:34PM +0200, Mickaël Salaün wrote:
quoted hunk ↗ jump to hunk
+/* 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.
quoted hunk ↗ jump to hunk
+ }
+ /* 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 &
quoted hunk ↗ jump to hunk
+ MNT_INTERNAL)) {
+ ret = ERR_PTR(-EINVAL);
+ goto put_fd;
What does it have to do with mountpoints, anyway?
quoted hunk ↗ jump to hunk
+/* 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?
quoted hunk ↗ jump to hunk
+ 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?