[RFC] bpf: Emit audit messages upon successful prog load and unload

[RFC] bpf: Emit audit messages upon successful prog load and unload

[Jiri Olsa]

From: Daniel Borkmann <daniel@iogearbox.net>

Allow for audit messages to be emitted upon BPF program load and
unload for having a timeline of events. The load itself is in
syscall context, so additional info about the process initiating
the BPF prog creation can be logged and later directly correlated
to the unload event.

The only info really needed from BPF side is the globally unique
prog ID where then audit user space tooling can query / dump all
info needed about the specific BPF program right upon load event
and enrich the record, thus these changes needed here can be kept
small and non-intrusive to the core.

Raw example output:

  # auditctl -D
  # auditctl -a always,exit -F arch=x86_64 -S bpf
  # ausearch --start recent -m 1334
  ...
  ----
  time->Wed Nov 27 16:04:13 2019
  type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf"
  type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321   \
    success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477    \
    pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001    \
    egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf"                \
    exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf"                  \
    subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
  type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD
  ----
  time->Wed Nov 27 16:04:13 2019
  type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 op=UNLOAD
  ...

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Co-developed-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/uapi/linux/audit.h |  1 +
 kernel/bpf/syscall.c       | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index c89c6495983d..32a5db900f47 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -116,6 +116,7 @@
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
 #define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
 #define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
+#define AUDIT_BPF		1334	/* BPF subsystem */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e3461ec59570..20826aad247c 100644
--- 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;
+	ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
+	if (unlikely(!ab))
+		return;
+	audit_log_format(ab, "prog-id=%u op=%s",
+			 prog->aux->id, bpf_audit_str[op]);
+	audit_log_end(ab);
+}
+
 int __bpf_prog_charge(struct user_struct *user, u32 pages)
 {
 	unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
@@ -1421,6 +1446,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
 {
 	if (atomic64_dec_and_test(&prog->aux->refcnt)) {
 		perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0);
+		bpf_audit_prog(prog, BPF_AUDIT_UNLOAD);
 		/* bpf_prog_free_id() must be called first */
 		bpf_prog_free_id(prog, do_idr_lock);
 		__bpf_prog_put_noref(prog, true);
@@ -1830,6 +1856,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 	 */
 	bpf_prog_kallsyms_add(prog);
 	perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0);
+	bpf_audit_prog(prog, BPF_AUDIT_LOAD);
 
 	err = bpf_prog_new_fd(prog);
 	if (err < 0)
-- 
2.23.0

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Jiri Olsa]

On Thu, Nov 28, 2019 at 10:16:32AM +0100, Jiri Olsa wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> 
> Allow for audit messages to be emitted upon BPF program load and
> unload for having a timeline of events. The load itself is in
> syscall context, so additional info about the process initiating
> the BPF prog creation can be logged and later directly correlated
> to the unload event.
> 
> The only info really needed from BPF side is the globally unique
> prog ID where then audit user space tooling can query / dump all
> info needed about the specific BPF program right upon load event
> and enrich the record, thus these changes needed here can be kept
> small and non-intrusive to the core.
> 
> Raw example output:
> 
>   # auditctl -D
>   # auditctl -a always,exit -F arch=x86_64 -S bpf
>   # ausearch --start recent -m 1334
>   ...
>   ----
>   time->Wed Nov 27 16:04:13 2019
>   type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf"
>   type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321   \
>     success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477    \
>     pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001    \
>     egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf"                \
>     exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf"                  \
>     subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
>   type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD
>   ----
>   time->Wed Nov 27 16:04:13 2019
>   type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 op=UNLOAD
>   ...
> 
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Co-developed-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

fyi I prepared userspace changes:
  https://github.com/olsajiri/audit-userspace/commit/3108a81fa8d937f07b4c78be8ae00fcd3d64f94d
  https://github.com/olsajiri/audit-testsuite/commit/16888ea7f14fa0269feef623d2a96f15f9ea71c9

I'll sumbit github PRs once the kernel change is pulled in

thanks,
jirka

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Paul Moore]

On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
>
> Allow for audit messages to be emitted upon BPF program load and
> unload for having a timeline of events. The load itself is in
> syscall context, so additional info about the process initiating
> the BPF prog creation can be logged and later directly correlated
> to the unload event.
>
> The only info really needed from BPF side is the globally unique
> prog ID where then audit user space tooling can query / dump all
> info needed about the specific BPF program right upon load event
> and enrich the record, thus these changes needed here can be kept
> small and non-intrusive to the core.
>
> Raw example output:
>
>   # auditctl -D
>   # auditctl -a always,exit -F arch=x86_64 -S bpf
>   # ausearch --start recent -m 1334
>   ...
>   ----
>   time->Wed Nov 27 16:04:13 2019
>   type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf"
>   type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321   \
>     success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477    \
>     pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001    \
>     egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf"                \
>     exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf"                  \
>     subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
>   type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD
>   ----
>   time->Wed Nov 27 16:04:13 2019
>   type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 op=UNLOAD
>   ...
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Co-developed-by: Jiri Olsa <jolsa@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/uapi/linux/audit.h |  1 +
>  kernel/bpf/syscall.c       | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)

Hi all, sorry for the delay; the merge window in combination with the
holiday in the US bumped this back a bit.  Small comments inline below
...

> --- 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(...)
}

> +       ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> +       if (unlikely(!ab))
> +               return;
> +       audit_log_format(ab, "prog-id=%u op=%s",
> +                        prog->aux->id, bpf_audit_str[op]);

Is it worth putting some checks in here to make sure that you don't
blow past the end of the bpf_audit_str array?

> +       audit_log_end(ab);
> +}

The audit record format looks much better now, thank you.  Although I
do wonder if you want bpf_audit_prog() to live in kernel/bpf/syscall.c
or in kernel/auditsc.c?  There is plenty of precedence for moving it
into auditsc.c and defining a no-op version for when
CONFIG_AUDITSYSCALL is not enabled, but I personally don't feel that
strongly about either option.  I just wanted to mention this in case
you weren't already aware.

If you do keep it in syscall.c, I don't think there is a need to
implement a no-op version dependent on CONFIG_AUDITSYSCALL; that will
just clutter the code.

If you do move it to auditsc.c please change the name to
audit_bpf()/__audit_bpf() so it matches the other functions; if you
keep it in syscall.c you can name it whatever you like :)

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Steve Grubb]

On Monday, December 2, 2019 6:00:14 PM EST Paul Moore wrote:
> On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > From: Daniel Borkmann <daniel@iogearbox.net>
> > 
> > Allow for audit messages to be emitted upon BPF program load and
> > unload for having a timeline of events. The load itself is in
> > syscall context, so additional info about the process initiating
> > the BPF prog creation can be logged and later directly correlated
> > to the unload event.
> > 
> > The only info really needed from BPF side is the globally unique
> > prog ID where then audit user space tooling can query / dump all
> > info needed about the specific BPF program right upon load event
> > and enrich the record, thus these changes needed here can be kept
> > small and non-intrusive to the core.
> > 
> > Raw example output:
> >   # auditctl -D
> >   # auditctl -a always,exit -F arch=x86_64 -S bpf
> >   # ausearch --start recent -m 1334
> >   ...
> >   ----
> >   time->Wed Nov 27 16:04:13 2019
> >   type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf"
> >   type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321
> >     \>   
> >     success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477 
> >       \
> >     pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001 
> >       \
> >     egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf"             
> >       \
> >     exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf"               
> >       \
> >     subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> >   
> >   type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD
> >   ----
> >   time->Wed Nov 27 16:04:13 2019
> >   type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76
> >   op=UNLOAD
> >   ...
> > 
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > Co-developed-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > 
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/bpf/syscall.c       | 27 +++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> 
> Hi all, sorry for the delay; the merge window in combination with the
> holiday in the US bumped this back a bit.  Small comments inline below
> ...
> 
> > --- 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(...)
> }
> 
> > +       ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> > +       if (unlikely(!ab))
> > +               return;
> > +       audit_log_format(ab, "prog-id=%u op=%s",
> > +                        prog->aux->id, bpf_audit_str[op]);
> 
> Is it worth putting some checks in here to make sure that you don't
> blow past the end of the bpf_audit_str array?

I am wondering if prog-id was really the only information that was needed? Is 
it meaningful to other tools? Does that correlate to anything in /proc? In 
earlier discussion, it sounded like more information was needed to be sure 
what was happening.

-Steve


> > +       audit_log_end(ab);
> > +}
> 
> The audit record format looks much better now, thank you.  Although I
> do wonder if you want bpf_audit_prog() to live in kernel/bpf/syscall.c
> or in kernel/auditsc.c?  There is plenty of precedence for moving it
> into auditsc.c and defining a no-op version for when
> CONFIG_AUDITSYSCALL is not enabled, but I personally don't feel that
> strongly about either option.  I just wanted to mention this in case
> you weren't already aware.
> 
> If you do keep it in syscall.c, I don't think there is a need to
> implement a no-op version dependent on CONFIG_AUDITSYSCALL; that will
> just clutter the code.
> 
> If you do move it to auditsc.c please change the name to
> audit_bpf()/__audit_bpf() so it matches the other functions; if you
> keep it in syscall.c you can name it whatever you like :)
> 
> --
> paul moore
> www.paul-moore.com

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Jiri Olsa]

On Mon, Dec 02, 2019 at 11:57:22PM -0500, Steve Grubb wrote:
> On Monday, December 2, 2019 6:00:14 PM EST Paul Moore wrote:
> > On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > From: Daniel Borkmann <daniel@iogearbox.net>
> > > 
> > > Allow for audit messages to be emitted upon BPF program load and
> > > unload for having a timeline of events. The load itself is in
> > > syscall context, so additional info about the process initiating
> > > the BPF prog creation can be logged and later directly correlated
> > > to the unload event.
> > > 
> > > The only info really needed from BPF side is the globally unique
> > > prog ID where then audit user space tooling can query / dump all
> > > info needed about the specific BPF program right upon load event
> > > and enrich the record, thus these changes needed here can be kept
> > > small and non-intrusive to the core.

SNIP

> > 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(...)
> > }
> > 
> > > +       ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> > > +       if (unlikely(!ab))
> > > +               return;
> > > +       audit_log_format(ab, "prog-id=%u op=%s",
> > > +                        prog->aux->id, bpf_audit_str[op]);
> > 
> > Is it worth putting some checks in here to make sure that you don't
> > blow past the end of the bpf_audit_str array?
> 
> I am wondering if prog-id was really the only information that was needed? Is 
> it meaningful to other tools? Does that correlate to anything in /proc? In 
> earlier discussion, it sounded like more information was needed to be sure 
> what was happening.

yep, as David mentions in the changelog the global ID is enough,
because you can get all the other bpf program info based on that

jirka

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Jiri Olsa]

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@kernel.org> wrote:
> > From: Daniel Borkmann <daniel@iogearbox.net>
> >
> > Allow for audit messages to be emitted upon BPF program load and
> > unload for having a timeline of events. The load itself is in
> > syscall context, so additional info about the process initiating
> > the BPF prog creation can be logged and later directly correlated
> > to the unload event.
> >
> > The only info really needed from BPF side is the globally unique
> > prog ID where then audit user space tooling can query / dump all
> > info needed about the specific BPF program right upon load event
> > and enrich the record, thus these changes needed here can be kept
> > small and non-intrusive to the core.
> >
> > Raw example output:
> >
> >   # auditctl -D
> >   # auditctl -a always,exit -F arch=x86_64 -S bpf
> >   # ausearch --start recent -m 1334
> >   ...
> >   ----
> >   time->Wed Nov 27 16:04:13 2019
> >   type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf"
> >   type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321   \
> >     success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477    \
> >     pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001    \
> >     egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf"                \
> >     exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf"                  \
> >     subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> >   type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD
> >   ----
> >   time->Wed Nov 27 16:04:13 2019
> >   type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 op=UNLOAD
> >   ...
> >
> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> > Co-developed-by: Jiri Olsa <jolsa@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/bpf/syscall.c       | 27 +++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> 
> Hi all, sorry for the delay; the merge window in combination with the
> holiday in the US bumped this back a bit.  Small comments inline below

np, thanks for review

> ...
> 
> > --- 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

I can see audit_log_start checks on value of audit_context() that we pass in,
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;
	...
}


> 
> > +       ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> > +       if (unlikely(!ab))
> > +               return;
> > +       audit_log_format(ab, "prog-id=%u op=%s",
> > +                        prog->aux->id, bpf_audit_str[op]);
> 
> Is it worth putting some checks in here to make sure that you don't
> blow past the end of the bpf_audit_str array?
> 
> > +       audit_log_end(ab);
> > +}
> 
> The audit record format looks much better now, thank you.  Although I
> do wonder if you want bpf_audit_prog() to live in kernel/bpf/syscall.c
> or in kernel/auditsc.c?  There is plenty of precedence for moving it
> into auditsc.c and defining a no-op version for when
> CONFIG_AUDITSYSCALL is not enabled, but I personally don't feel that
> strongly about either option.  I just wanted to mention this in case
> you weren't already aware.
> 
> If you do keep it in syscall.c, I don't think there is a need to
> implement a no-op version dependent on CONFIG_AUDITSYSCALL; that will
> just clutter the code.
> 
> If you do move it to auditsc.c please change the name to
> audit_bpf()/__audit_bpf() so it matches the other functions; if you
> keep it in syscall.c you can name it whatever you like :)

ok, so far I think we'll keep it kernel/bpf/syscall.c

thanks,
jirka

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Paul Moore]

On Tue, Dec 3, 2019 at 4:38 AM Jiri Olsa <jolsa@redhat.com> 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@kernel.org> 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

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Jiri Olsa]

On Tue, Dec 03, 2019 at 09:53:16PM -0500, Paul Moore wrote:

SNIP

> > >
> > > 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).

ok, so something like below?

thanks,
jirka


---
 include/uapi/linux/audit.h |  1 +
 kernel/bpf/syscall.c       | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index c89c6495983d..32a5db900f47 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -116,6 +116,7 @@
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
 #define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
 #define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
+#define AUDIT_BPF		1334	/* BPF subsystem */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index e3461ec59570..81f1a6308aa8 100644
--- 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,33 @@ 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_context *ctx = NULL;
+	struct audit_buffer *ab;
+
+	if (audit_enabled == AUDIT_OFF)
+		return;
+	if (op == BPF_AUDIT_LOAD)
+		ctx = audit_context();
+	ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF);
+	if (unlikely(!ab))
+		return;
+	audit_log_format(ab, "prog-id=%u op=%s",
+			 prog->aux->id, bpf_audit_str[op]);
+	audit_log_end(ab);
+}
+
 int __bpf_prog_charge(struct user_struct *user, u32 pages)
 {
 	unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
@@ -1421,6 +1449,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
 {
 	if (atomic64_dec_and_test(&prog->aux->refcnt)) {
 		perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0);
+		bpf_audit_prog(prog, BPF_AUDIT_UNLOAD);
 		/* bpf_prog_free_id() must be called first */
 		bpf_prog_free_id(prog, do_idr_lock);
 		__bpf_prog_put_noref(prog, true);
@@ -1830,6 +1859,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
 	 */
 	bpf_prog_kallsyms_add(prog);
 	perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0);
+	bpf_audit_prog(prog, BPF_AUDIT_LOAD);
 
 	err = bpf_prog_new_fd(prog);
 	if (err < 0)
-- 
2.23.0

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Paul Moore]

On Wed, Dec 4, 2019 at 9:08 AM Jiri Olsa <jolsa@redhat.com> wrote:
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index c89c6495983d..32a5db900f47 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -116,6 +116,7 @@
>  #define AUDIT_FANOTIFY         1331    /* Fanotify access decision */
>  #define AUDIT_TIME_INJOFFSET   1332    /* Timekeeping offset injected */
>  #define AUDIT_TIME_ADJNTPVAL   1333    /* NTP value adjustment */
> +#define AUDIT_BPF              1334    /* BPF subsystem */
>
>  #define AUDIT_AVC              1400    /* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR      1401    /* Internal SE Linux Errors */
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index e3461ec59570..81f1a6308aa8 100644
> --- 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,33 @@ 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_context *ctx = NULL;
> +       struct audit_buffer *ab;
> +
> +       if (audit_enabled == AUDIT_OFF)
> +               return;
> +       if (op == BPF_AUDIT_LOAD)
> +               ctx = audit_context();
> +       ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF);
> +       if (unlikely(!ab))
> +               return;
> +       audit_log_format(ab, "prog-id=%u op=%s",
> +                        prog->aux->id, bpf_audit_str[op]);
> +       audit_log_end(ab);
> +}

As mentioned previously, I still think it might be a good idea to
ensure "op" is within the bounds of bpf_audit_str, but the audit bits
look reasonable to me.

>  int __bpf_prog_charge(struct user_struct *user, u32 pages)
>  {
>         unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> @@ -1421,6 +1449,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock)
>  {
>         if (atomic64_dec_and_test(&prog->aux->refcnt)) {
>                 perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0);
> +               bpf_audit_prog(prog, BPF_AUDIT_UNLOAD);
>                 /* bpf_prog_free_id() must be called first */
>                 bpf_prog_free_id(prog, do_idr_lock);
>                 __bpf_prog_put_noref(prog, true);
> @@ -1830,6 +1859,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr)
>          */
>         bpf_prog_kallsyms_add(prog);
>         perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0);
> +       bpf_audit_prog(prog, BPF_AUDIT_LOAD);
>
>         err = bpf_prog_new_fd(prog);
>         if (err < 0)
> --
> 2.23.0

-- 
paul moore
www.paul-moore.com

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Jiri Olsa]

On Wed, Dec 04, 2019 at 09:38:10AM -0500, Paul Moore wrote:

SNIP

> > +
> > +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_context *ctx = NULL;
> > +       struct audit_buffer *ab;
> > +
> > +       if (audit_enabled == AUDIT_OFF)
> > +               return;
> > +       if (op == BPF_AUDIT_LOAD)
> > +               ctx = audit_context();
> > +       ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF);
> > +       if (unlikely(!ab))
> > +               return;
> > +       audit_log_format(ab, "prog-id=%u op=%s",
> > +                        prog->aux->id, bpf_audit_str[op]);
> > +       audit_log_end(ab);
> > +}
> 
> As mentioned previously, I still think it might be a good idea to
> ensure "op" is within the bounds of bpf_audit_str, but the audit bits
> look reasonable to me.

ok, I'll add that, I'll send out full patch

thanks for the review,
jirka

Re: [RFC] bpf: Emit audit messages upon successful prog load and unload

[Jiri Olsa]

On Mon, Dec 02, 2019 at 06:00:14PM -0500, Paul Moore wrote:

SNIP

> > +
> > +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(...)
> }
> 
> > +       ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> > +       if (unlikely(!ab))
> > +               return;
> > +       audit_log_format(ab, "prog-id=%u op=%s",
> > +                        prog->aux->id, bpf_audit_str[op]);
> 
> Is it worth putting some checks in here to make sure that you don't
> blow past the end of the bpf_audit_str array?

forgot answer this one..  there are only 2 callers:

  bpf_audit_prog(prog, BPF_AUDIT_UNLOAD);
  bpf_audit_prog(prog, BPF_AUDIT_LOAD);

that's not going to change any time soon,
so I dont think we don't need such check

jirka