[RFC] misuse of descriptor tables in HID-BPF

[RFC] misuse of descriptor tables in HID-BPF

[Al Viro]

static int hid_bpf_insert_prog(int prog_fd, struct bpf_prog *prog)
{
        int i, index = -1, map_fd = -1, err = -EINVAL;

        /* retrieve a fd of our prog_array map in BPF */
        map_fd = skel_map_get_fd_by_id(jmp_table.map->id);

	...

        /* insert the program in the jump table */
        err = skel_map_update_elem(map_fd, &index, &prog_fd, 0);
        if (err)
                goto out;

	...

        if (err < 0)
                __hid_bpf_do_release_prog(map_fd, index);
        if (map_fd >= 0)
                close_fd(map_fd);
        return err;
}

What.  The.  Hell?

Folks, descriptor table is a shared object.  It is NOT safe to use
as a scratchpad.

Another thread might do whatever it bloody wants to anything inserted
there.  It may close your descriptor, it may replace it with something
entirely unrelated to what you've placed there, etc.

This is fundamentally broken.  The same goes for anything that tries
to play similar games.  Don't use descriptor tables that way.

Kernel-side descriptors should be used only for marshalling - they can
be passed by userland (and should be resolved to struct file *, with
no expectations that repeated call of fget() would yield the same
pointer) and they can be returned _to_ userland - after you've allocated
them and associated them with struct file *.

Using them as handles for internal objects is an equivalent of playing
in the traffic - think of it as evolution in action.

Re: [RFC] misuse of descriptor tables in HID-BPF

[Benjamin Tissoires]

Hi Al,

On Jun 03 2024, Al Viro wrote:
> static int hid_bpf_insert_prog(int prog_fd, struct bpf_prog *prog)
> {
>         int i, index = -1, map_fd = -1, err = -EINVAL;
> 
>         /* retrieve a fd of our prog_array map in BPF */
>         map_fd = skel_map_get_fd_by_id(jmp_table.map->id);
> 
> 	...
> 
>         /* insert the program in the jump table */
>         err = skel_map_update_elem(map_fd, &index, &prog_fd, 0);
>         if (err)
>                 goto out;
> 
> 	...
> 
>         if (err < 0)
>                 __hid_bpf_do_release_prog(map_fd, index);
>         if (map_fd >= 0)
>                 close_fd(map_fd);
>         return err;
> }
> 
> What.  The.  Hell?

TL;DR: I'm getting rid of that code in (hopefully v6.11), so any bad
justification I can come up with can safely be ignored.

> 
> Folks, descriptor table is a shared object.  It is NOT safe to use
> as a scratchpad.
> 
> Another thread might do whatever it bloody wants to anything inserted
> there.  It may close your descriptor, it may replace it with something
> entirely unrelated to what you've placed there, etc.
> 
> This is fundamentally broken.  The same goes for anything that tries
> to play similar games.  Don't use descriptor tables that way.
> 
> Kernel-side descriptors should be used only for marshalling - they can
> be passed by userland (and should be resolved to struct file *, with
> no expectations that repeated call of fget() would yield the same
> pointer) and they can be returned _to_ userland - after you've allocated
> them and associated them with struct file *.

I think the situation is not as bad as you think it is.
First these fds are not actually struct file * but bpf objects. So the
users of them should be rather small. Threading in this case is probably
fine because this only happens under a mutex lock.

And last, and I think that's the biggest point here, we are not really
"in the kernel". The code is executed in the kernel, but it's coming
from a syscall bpf program, and as such we are in a blurry state between
userspace and kernel space. Then, the bpf API uses fds because we are
effectively going through the userspace API from within the kernel to
populate the maps. At any point here we don't assume the fd is safe.
Whenever the call to skel_map_update_elem() is done, the kernel emits
an equivalent of a syscall to this kernel function, and as such the
actual kernel code behind it treats everything like a kernel code should.

Efectively, this use of fds is to pass information from/to userland :)

But I agree, the whole logic of this file is weird, and not clean.

Luckily, with the help of the bpf folks, I came to a better solution,
with bpf_struct_ops[0].

> 
> Using them as handles for internal objects is an equivalent of playing
> in the traffic - think of it as evolution in action.

Which now raises the question: should I ask for the bpf_struct_ops
conversion to be backported in stable, because that code is too ugly?

Cheers,
Benjamin

[0] https://lore.kernel.org/bpf/20240528-hid_bpf_struct_ops-v1-0-8c6663df27d8@kernel.org/

Re: [RFC] misuse of descriptor tables in HID-BPF

[Al Viro]

On Mon, Jun 03, 2024 at 04:46:31PM +0200, Benjamin Tissoires wrote:

> > Kernel-side descriptors should be used only for marshalling - they can
> > be passed by userland (and should be resolved to struct file *, with
> > no expectations that repeated call of fget() would yield the same
> > pointer) and they can be returned _to_ userland - after you've allocated
> > them and associated them with struct file *.
> 
> I think the situation is not as bad as you think it is.
> First these fds are not actually struct file * but bpf objects. So the
> users of them should be rather small. Threading in this case is probably
> fine because this only happens under a mutex lock.

First of all, _any_ of those is actually a struct file * - it's just that
initially you have a reference to opened bpffs file there.  As soon
as it's in the table, it can be replaced with _any_ reference to opened
file.  And no, your mutex doesn't matter here - dup2() neither knows
nor cares about it.  And that's all you need to replace a descriptor table
entry - dup2() from another thread that happens to share descriptor table
with the one you are currently running in.

> And last, and I think that's the biggest point here, we are not really
> "in the kernel". The code is executed in the kernel, but it's coming
> from a syscall bpf program, and as such we are in a blurry state between
> userspace and kernel space.

In context of which thread is it executed?  IOW, what does current point
to when your code runs?

> Then, the bpf API uses fds because we are
> effectively going through the userspace API from within the kernel to
> populate the maps. At any point here we don't assume the fd is safe.
> Whenever the call to skel_map_update_elem() is done, the kernel emits
> an equivalent of a syscall to this kernel function, and as such the
> actual kernel code behind it treats everything like a kernel code should.

It's not about the isolated accesses of descriptor table being unsafe;
it's about the assumption that references you've put into descriptor
table will not be replaced by another thread, both sides using perfectly
sound operations for each access.

Again, descriptor table should be treated as a shared data structure.
There are some exceptions (e.g. in execve() past the point of no return,
after it has explicitly unshared the descriptor table), but none of those
applies in your case.

There's no such thing as "lock descriptor table against modifications"
available outside of fs/file.c; moreover, the thing used inside fs/file.c
to protect individual table modifications is a spinlock and it really
can't be held over blocking operations.

So the descriptor table itself will be fine, but the references you store
in there might be replaced at any point.

> Efectively, this use of fds is to pass information from/to userland :)

"Here's a new descriptor, feel free to do IO on it" is fine - if another
thread starts playing silly buggers with descriptor table (e.g. closes
that descriptor behind the first thread's back), it's a userland bug
and the kernel is fine.  Two threads sharing descriptor table are likely
to share memory as well, so they can step on each others' toes in a lot
of other ways.  Your situation is different - you have kernel-side code
putting stuff into descriptor table and expecting the same thing to be
found at the same place later on.  _That_ is a trouble waiting to happen.

> But I agree, the whole logic of this file is weird, and not clean.
> 
> Luckily, with the help of the bpf folks, I came to a better solution,
> with bpf_struct_ops[0].

Hadn't looked at it yet...

Re: [RFC] misuse of descriptor tables in HID-BPF

[Benjamin Tissoires]

On Jun 03 2024, Al Viro wrote:
> On Mon, Jun 03, 2024 at 04:46:31PM +0200, Benjamin Tissoires wrote:
> 
> > > Kernel-side descriptors should be used only for marshalling - they can
> > > be passed by userland (and should be resolved to struct file *, with
> > > no expectations that repeated call of fget() would yield the same
> > > pointer) and they can be returned _to_ userland - after you've allocated
> > > them and associated them with struct file *.
> > 
> > I think the situation is not as bad as you think it is.
> > First these fds are not actually struct file * but bpf objects. So the
> > users of them should be rather small. Threading in this case is probably
> > fine because this only happens under a mutex lock.
> 
> First of all, _any_ of those is actually a struct file * - it's just that
> initially you have a reference to opened bpffs file there.  As soon
> as it's in the table, it can be replaced with _any_ reference to opened
> file.  And no, your mutex doesn't matter here - dup2() neither knows
> nor cares about it.  And that's all you need to replace a descriptor table
> entry - dup2() from another thread that happens to share descriptor table
> with the one you are currently running in.

I think I'm still confused by what you said. Are you talking about
map_fd or prog_fd in the code extract you took (or both)?

FWIW, this part of the code will get removed entirely with struct_ops,
so a "not enough time to explain to an idiot person" is a perfectly fine
answer :)

map_fd is a fd to a map in a bpf preloaded by the kernel at boot time.
The kernel keeps a reference of it, and won't release it, thus my mutex,
which prevents the map to be removed while hid_bpf_attach(), is called.
Yes it won't prevent dup2(), but at least that ensures that struct
bpf_map * is safe.

prog_fd is coming from userspace, and we are in a bpf syscall here.
So if userspace wants to shoot itself in the foot, that's fine, but once
we re-enter the kernel (skel_map_update_elem()), bpf-core ensures that
prog_fd is actually pointing to a bpf object (or a struct file* pointing
at a bpf object).

Nowhere we are dealing with bpffs here. The bpf_prog is not pinned in
the bpffs. But maybe you are talking about the internal mapping bpf<->fd
that bpf-core does:
When skel_map_get_fd_by_id() is called, we effectively call
kern_sys_bpf(BPF_MAP_GET_FD_BY_ID), which goes through the same checks
than the syscall __NR_BPF. Then internally, this translates to
anon_inode_getfd().

Now, regarding dup2(): if between skel_map_get_fd_by_id() and close_fd(map_fd);
userspace calls dup2() to replace the the internal fd of the map by
another opened file, both calls to skel_map_update_elem() and
__hid_bpf_do_release_prog() are safe because they do the translation
into struct bpf_map* and check that they are dealing with a bpf_map.

If dup2() happens between skel_map_update_elem() and
__hid_bpf_do_release_prog(), that's fine too: there is an error, so the
index will eventually get reused, and the struct bpf_prog will
eventually be released. But it will never be used because the error is
known to hid_bpf_jmp_table.c.

So efectively dup2() is a userspace bug because all what happens is that you
will get an error and nothing will happen in the kernel (bonus point:
your newly reassigned fd will be closed).

> 
> > And last, and I think that's the biggest point here, we are not really
> > "in the kernel". The code is executed in the kernel, but it's coming
> > from a syscall bpf program, and as such we are in a blurry state between
> > userspace and kernel space.
> 
> In context of which thread is it executed?  IOW, what does current point
> to when your code runs?

As I understand it, current will point to the thread context of the caller.
The various fds I can retrieve here are the exact same numbers from
userspace or in the bpf syscall. I'm not sure I can do a simple printk()
on current, but if you have a simple pseudo code extract for me to get the
information you need I can probably run it.

> 
> > Then, the bpf API uses fds because we are
> > effectively going through the userspace API from within the kernel to
> > populate the maps. At any point here we don't assume the fd is safe.
> > Whenever the call to skel_map_update_elem() is done, the kernel emits
> > an equivalent of a syscall to this kernel function, and as such the
> > actual kernel code behind it treats everything like a kernel code should.
> 
> It's not about the isolated accesses of descriptor table being unsafe;
> it's about the assumption that references you've put into descriptor
> table will not be replaced by another thread, both sides using perfectly
> sound operations for each access.
> 
> Again, descriptor table should be treated as a shared data structure.
> There are some exceptions (e.g. in execve() past the point of no return,
> after it has explicitly unshared the descriptor table), but none of those
> applies in your case.
> 
> There's no such thing as "lock descriptor table against modifications"
> available outside of fs/file.c; moreover, the thing used inside fs/file.c
> to protect individual table modifications is a spinlock and it really
> can't be held over blocking operations.
> 
> So the descriptor table itself will be fine, but the references you store
> in there might be replaced at any point.

Again I don't disagree with you on the general usage, but I don't see
how I am going against.

I am not storing any fd anywhere. I call a UAPI indirect call from the
kernel which will again do a translation fd->bpf_map and fd->bpf_prog
into this bpf_map, with all of the checks that are required to see if
the pointer obtained from the fd is of the correct type.

Everything that stays and is stored in the kernel is the actual struct
bpf_prog* or struct bpf_map*, with refcounting done properly (AFAICT for
the refcounting).

> 
> > Efectively, this use of fds is to pass information from/to userland :)
> 
> "Here's a new descriptor, feel free to do IO on it" is fine - if another
> thread starts playing silly buggers with descriptor table (e.g. closes
> that descriptor behind the first thread's back), it's a userland bug
> and the kernel is fine.  Two threads sharing descriptor table are likely
> to share memory as well, so they can step on each others' toes in a lot
> of other ways.  Your situation is different - you have kernel-side code
> putting stuff into descriptor table and expecting the same thing to be
> found at the same place later on.  _That_ is a trouble waiting to happen.

If the descriptor is changed during that syscall operation because of dup2(),
it's fine because it's never stored as it is, and skel_map_update_elem() or
__hid_bpf_do_release_prog() will convert the descriptors into kernel
struct and will reject wrong types.

And if the problem is because I call skel_map_get_fd_by_id() and then
close_fd() at the end, I don't see the problem because that fd is never
shared with userspace.

> 
> > But I agree, the whole logic of this file is weird, and not clean.
> > 
> > Luckily, with the help of the bpf folks, I came to a better solution,
> > with bpf_struct_ops[0].
> 
> Hadn't looked at it yet...

From a fd point of view, this is much simpler (or it is punted to
bpf-core): there are none :)

So no need to review it if you don't have time, I'm basically entirely
rewriting this code, removing all of those maps and indirections, by
just keeping a list of proper refcounted kernel struct bpf_struct_ops *.

Cheers,
Benjamin