On Tue, Dec 3, 2019 at 4:38 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> On Mon, Dec 02, 2019 at 06:00:14PM -0500, Paul Moore wrote:
> > On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
...
> > > --- a/kernel/bpf/syscall.c
> > > +++ b/kernel/bpf/syscall.c
> > > @@ -23,6 +23,7 @@
> > > #include <linux/timekeeping.h>
> > > #include <linux/ctype.h>
> > > #include <linux/nospec.h>
> > > +#include <linux/audit.h>
> > > #include <uapi/linux/btf.h>
> > >
> > > #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \
> > > @@ -1306,6 +1307,30 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog)
> > > return 0;
> > > }
> > >
> > > +enum bpf_audit {
> > > + BPF_AUDIT_LOAD,
> > > + BPF_AUDIT_UNLOAD,
> > > +};
> > > +
> > > +static const char * const bpf_audit_str[] = {
> > > + [BPF_AUDIT_LOAD] = "LOAD",
> > > + [BPF_AUDIT_UNLOAD] = "UNLOAD",
> > > +};
> > > +
> > > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op)
> > > +{
> > > + struct audit_buffer *ab;
> > > +
> > > + if (audit_enabled == AUDIT_OFF)
> > > + return;
> >
> > I think you would probably also want to check the results of
> > audit_dummy_context() here as well, see all the various audit_XXX()
> > functions in include/linux/audit.h as an example. You'll see a
> > pattern similar to the following:
> >
> > static inline void audit_foo(...)
> > {
> > if (unlikely(!audit_dummy_context()))
> > __audit_foo(...)
> > }
>
> bpf_audit_prog might be called outside of syscall context for UNLOAD event,
> so that would prevent it from being stored
Okay, right. More on this below ...
> I can see audit_log_start checks on value of audit_context() that we pass in,
The check in audit_log_start() is for a different reason; it checks
the passed context to see if it should associate the record with
others in the same event, e.g. PATH records associated with the
matching SYSCALL record. This way all the associated records show up
as part of the same event (as defined by the audit timestamp).
> should we check for audit_dummy_context just for load event? like:
>
>
> static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op)
> {
> struct audit_buffer *ab;
>
> if (audit_enabled == AUDIT_OFF)
> return;
> if (op == BPF_AUDIT_LOAD && audit_dummy_context())
> return;
> ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> if (unlikely(!ab))
> return;
> ...
> }
Ignoring the dummy context for a minute, there is likely a larger
issue here with using audit_context() when bpf_audit_prog() is called
outside of a syscall, e.g. BPF_AUDIT_UNLOAD. In this case we likely
shouldn't be taking the audit context from the current task, we
shouldn't be taking it from anywhere, it should be NULL.
As far as the dummy context is concerned, you might want to skip the
dummy context check since you can only do that on the LOAD side, which
means that depending on the system's configuration you could end up
with a number of unbalanced LOAD/UNLOAD events. The downside is that
you are always going to get the BPF audit records on systemd based
systems, since they ignore the admin's audit configuration and always
enable audit (yes, we've tried to get systemd to change, they don't
seem to care).
--
paul moore
www.paul-moore.com
