Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Jun 10, 2020 at 04:12:29PM -0500, Eric W. Biederman wrote:
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> 
> > On Tue, Jun 09, 2020 at 03:02:30PM -0500, Eric W. Biederman wrote:
> >> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> >> 
> >> > bpf_lsm is that thing that needs to load and start acting early.
> >> > It's somewhat chicken and egg. fork_usermode_blob() will start a process
> >> > that will load and apply security policy to all further forks and
> >> > execs.
> >> 
> >> What is the timeframe for bpf_lsm patches wanting to use
> >> fork_usermode_blob()?
> >> 
> >> Are we possibly looking at something that will be ready for the next
> >> merge window?
> >
> > In bpf space there are these that want to use usermode_blobs:
> > 1. bpfilter itself.
> > First of all I think we made a mistake delaying landing the main patches:
> > https://lore.kernel.org/patchwork/patch/902785/
> > https://lore.kernel.org/patchwork/patch/902783/
> > without them bpfilter is indeed dead. That probably was the reason
> > no one was brave enough to continue working on it.
> > So I think the landed skeleton of bpfilter can be removed.
> > I think no user space code will notice that include/uapi/linux/bpfilter.h
> > is gone. So it won't be considered as user space breakage.
> > Similarly CONFIG_BPFILTER can be nuked too.
> > bpftool is checking for it (see tools/bpf/bpftool/feature.c)
> > but it's fine to remove it.
> > I still think that the approach taken was a correct one, but
> > lifting that project off the ground was too much for three of us.
> > So when it's staffed appropriately we can re-add that code.
> >
> > 2. bpf_lsm.
> > It's very active at the moment. I'm working on it as well
> > (sleepable progs is targeting that), but I'm not sure when folks
> > would have to have it during the boot. So far it sounds that
> > they're addressing more critical needs first. "bpf_lsm ready at boot"
> > came up several times during "bpf office hours" conference calls,
> > so it's certainly on the radar. If I to guess I don't think
> > bpf_lsm will use usermode_blobs in the next 6 weeks.
> > More likely 2-4 month.
> >
> > 3. bpf iterator.
> > It's already capable extension of several things in /proc.
> > See https://lore.kernel.org/bpf/20200509175921.2477493-1-yhs@fb.com/
> > Cat-ing bpf program as "cat /sys/fs/bpf/my_ipv6_route"
> > will produce the same human output as "cat /proc/net/ipv6_route".
> > The key difference is that bpf is all tracing based and it's unstable.
> > struct fib6_info can change and prog will stop loading.
> > There are few FIXME in there. That is being addressed right now.
> > After that the next step is to make cat-able progs available
> > right after boot via usermode_blobs.
> > Unlike cases 1 and 2 here we don't care that they appear before pid 1.
> > They can certainly be chef installed and started as services.
> > But they are kernel dependent, so deploying them to production
> > is much more complicated when they're done as separate rpm.
> > Testing is harder and so on. Operational issues pile up when something
> > that almost like kernel module is done as a separate package.
> > Hence usermode_blob fits the best.
> > Of course we were not planning to add a bunch of them to kernel tree.
> > The idea was to add only _one_ such cat-able bpf prog and have it as
> > a selftest for usermode_blob + bpf_iter. What we want our users to
> > see in 'cat my_ipv6_route' is probably different from other companies.
> > These patches will likely be using usermode_blob() in the next month.
> >
> > But we don't need to wait. We can make the progress right now.
> > How about we remove bpfilter uapi and rename net/bpfilter/bpfilter_kern.c
> > into net/umb/umb_test.c only to exercise Makefile to build elf file
> > from simple main.c including .S with incbin trick
> > and kernel side that does fork_usermode_blob().
> > And that's it.
> > net/ipv4/bpfilter/sockopt.c and kconfig can be removed.
> > That would be enough base to do use cases 2 and 3 above.
> > Having such selftest will be enough to adjust the layering
> > for fork_usermode_blob(), right?
> 
> If I understand correctly you are asking people to support out of tree
> code.  I see some justification for this functionality for in-tree code.
> For out of tree code there really is no way to understand support or
> maintain the code.

It's just like saying that sys_finit_module() is there to support out
of tree code. There are in- and out- tree modules and there will be
in- and out- of tree bpf programs, but the focus is on those that
are relevant for the long term future of the kernel.
The 1 case above is in-tree only. There is nothing in bpfilter
that makes sense out of tree.
The 2 case (bpf_lsm) is primarily in-tree. Security is something
everyone wants its own way, but majority of bpf_lsm functionality
should live in-tree.
The 3 case is mostly out-of-tree. If there was obvious way to
extend /proc it could have been in-tree, but no one will agree.

> We probably also need to have a conversation about why this
> functionality is a better choice that using a compiled in initramfs,
> such as can be had by setting CONFIG_INITRAMFS_SOURCE.

I explained it several times already. I don't see how initramfs solves 1 and 2.

> Even with this write up and the conversations so far I don't understand
> what problem fork_usermode_blob is supposed to be solving.  Is there
> anything kernel version dependent about bpf_lsm?  For me the primary
> justification of something like fork_usermode_blob is something that is
> for all practical purposes a kernel module but it just happens to run in
> usermode.

that's what it is. It's a kernel module that runs in user space.

> From what little I know about bpf_lsm that isn't the case.  So far all

It is.

> you have mentioned is that bpf_lsm needs to load early.  That seems like
> something that could be solved by a couple of lines init/main.c that
> forks and exec's a program before init if it is present.  Maybe that
> also needs a bit of protection so the bootloader can't override the
> binary.
> 
> The entire concept of a loadable lsm has me scratching my head.  Last
> time that concept was seriously looked at the races for initializing per
> object data were difficult enough to deal with modular support was
> removed from all of the existing lsms.

I'm not sure what races you're talking about.
usermode_blob will interact with kernel via syscalls and other standard
communication mechanism.

> Not to mention there are places where the lsm hooks are a pretty lousy
> API and will be refactored to make things better with no thought of any
> out of tree code.

I don't see how refactoring LSM hooks is relevant in this discussion.

> 
> > If I understood you correctly you want to replace pid_t
> > in 'struct umh_info' with proper 'struct pid' pointer that
> > is refcounted, so user process's exit is clean? What else?
> 
> No "if (filename)" or "if (file)" on the exec code paths.  No extra case
> for the LSM's to have to deal with.  Nothing fork_usermode_blob does is
> something that can't be done from userspace as far as execve is
> concerned so there is no justification for any special cases in the core
> of the exec code.

Adding getname_kernel() instead of filename==NULL is trivial enough
and makes sense as a cleanup.
But where do you see 'if (file)' ?
The correct 'file' pointer is passed from shmem_kernel_file_setup() all
the way to exec.

> Getting the deny_write_count and the reference count correct on the file
> argument as well as getting BPRM_FLAGS_PATH_INACCESSIBLE set.

There is no fd because there is no task, but there is a file. I think 
do_execve should assume BINPRM_FLAGS_PATH_INACCESSIBLE in this case.

> Using the proper type for argv and envp.

I guess that's going to be a part of other cleanup.

> Those are the things I know of that need to be addressed.
> 
> 
> Getting the code refactored so that the do_open_execat can be called
> in do_execveat_common instead of __do_execve_file is enough of a
> challenge of code motion I really would rather not do that.   Unfortunately that is
> the only way I can see right now to have both do_execveat_common and
> do_execve_file pass in a struct file.

The 'struct file' is there. Please take another look at the code.

> Calling deny_write_access and get_file in do_execve_file and probably
> a bit more is the only way I can see to cleanly isoloate the special
> cases fork_usermode_blob brings to the table.
> 
> 
> Strictly speaking I am also aware of the issue that the kernel has to
> use set_fs(KERNEL_DS) to allow argv and envp to exist in kernel space
> instead of userspace.  That needs to be fixed as well, but for all
> kernel uses of exec.  So any work fixing fork_usermode_blob can ignore
> that issue.

well, this is the problem of usermodehelper_exec.
usermode_blob doesn't use argv/envp.
They could be NULL for all practical purpose.