netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC] bpf: Emit audit messages upon successful prog load and unload
@ 2019-11-28  9:16 Jiri Olsa
  2019-11-28  9:18 ` Jiri Olsa
  2019-12-02 23:00 ` Paul Moore
  0 siblings, 2 replies; 14+ messages in thread
From: Jiri Olsa @ 2019-11-28  9:16 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: netdev, bpf, linux-audit, Andrii Nakryiko, Yonghong Song,
	Martin KaFai Lau, Jakub Kicinski, Steve Grubb, David Miller,
	Paul Moore, Eric Paris, Jiri Benc

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-11-28  9:16 [RFC] bpf: Emit audit messages upon successful prog load and unload Jiri Olsa
@ 2019-11-28  9:18 ` Jiri Olsa
  2019-12-02 23:00 ` Paul Moore
  1 sibling, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2019-11-28  9:18 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, linux-audit,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Jakub Kicinski,
	Steve Grubb, David Miller, Paul Moore, Eric Paris, Jiri Benc

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-11-28  9:16 [RFC] bpf: Emit audit messages upon successful prog load and unload Jiri Olsa
  2019-11-28  9:18 ` Jiri Olsa
@ 2019-12-02 23:00 ` Paul Moore
  2019-12-03  4:57   ` Steve Grubb
                     ` (2 more replies)
  1 sibling, 3 replies; 14+ messages in thread
From: Paul Moore @ 2019-12-02 23:00 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, linux-audit,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Jakub Kicinski,
	Steve Grubb, David Miller, Eric Paris, Jiri Benc

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

--
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-02 23:00 ` Paul Moore
@ 2019-12-03  4:57   ` Steve Grubb
  2019-12-03  8:46     ` Jiri Olsa
  2019-12-03  9:38   ` Jiri Olsa
  2019-12-04 14:02   ` Jiri Olsa
  2 siblings, 1 reply; 14+ messages in thread
From: Steve Grubb @ 2019-12-03  4:57 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	linux-audit, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, David Miller, Eric Paris, Jiri Benc

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





^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-03  4:57   ` Steve Grubb
@ 2019-12-03  8:46     ` Jiri Olsa
  0 siblings, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2019-12-03  8:46 UTC (permalink / raw)
  To: Steve Grubb
  Cc: Paul Moore, Jiri Olsa, Alexei Starovoitov, Daniel Borkmann,
	netdev, bpf, linux-audit, Andrii Nakryiko, Yonghong Song,
	Martin KaFai Lau, Jakub Kicinski, David Miller, Eric Paris,
	Jiri Benc

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-02 23:00 ` Paul Moore
  2019-12-03  4:57   ` Steve Grubb
@ 2019-12-03  9:38   ` Jiri Olsa
  2019-12-04  2:53     ` Paul Moore
  2019-12-04 14:02   ` Jiri Olsa
  2 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2019-12-03  9:38 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	linux-audit, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, Steve Grubb, David Miller, Eric Paris, Jiri Benc

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-03  9:38   ` Jiri Olsa
@ 2019-12-04  2:53     ` Paul Moore
  2019-12-04 14:08       ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Paul Moore @ 2019-12-04  2:53 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	linux-audit, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, Steve Grubb, David Miller, Eric Paris, Jiri Benc

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-02 23:00 ` Paul Moore
  2019-12-03  4:57   ` Steve Grubb
  2019-12-03  9:38   ` Jiri Olsa
@ 2019-12-04 14:02   ` Jiri Olsa
  2 siblings, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2019-12-04 14:02 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	linux-audit, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, Steve Grubb, David Miller, Eric Paris, Jiri Benc

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-04  2:53     ` Paul Moore
@ 2019-12-04 14:08       ` Jiri Olsa
  2019-12-04 14:38         ` Paul Moore
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2019-12-04 14:08 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	linux-audit, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, Steve Grubb, David Miller, Eric Paris, Jiri Benc

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-04 14:08       ` Jiri Olsa
@ 2019-12-04 14:38         ` Paul Moore
  2019-12-04 15:26           ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Paul Moore @ 2019-12-04 14:38 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	linux-audit, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, Steve Grubb, David Miller, Eric Paris, Jiri Benc

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: Emit audit messages upon successful prog load and unload
  2019-12-04 14:38         ` Paul Moore
@ 2019-12-04 15:26           ` Jiri Olsa
  0 siblings, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2019-12-04 15:26 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	linux-audit, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, Steve Grubb, David Miller, Eric Paris, Jiri Benc

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: emit audit messages upon successful prog load and unload
  2019-11-20 21:14 ` Alexei Starovoitov
@ 2019-11-20 21:30   ` Jiri Olsa
  0 siblings, 0 replies; 14+ messages in thread
From: Jiri Olsa @ 2019-11-20 21:30 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Jakub Kicinski,
	Steve Grubb, David Miller, Paul Moore, Eric Paris, Jiri Benc

On Wed, Nov 20, 2019 at 01:14:40PM -0800, Alexei Starovoitov wrote:
> On Wed, Nov 20, 2019 at 03:38:10PM +0100, Jiri Olsa wrote:
> > 
> > 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.
> 
> ...
> 
> > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_event event)
> > +{
> > +	bool has_task_context = event == BPF_EVENT_LOAD;
> > +	struct audit_buffer *ab;
> > +
> > +	if (audit_enabled == AUDIT_OFF)
> > +		return;
> > +	ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> > +	if (unlikely(!ab))
> > +		return;
> > +	if (has_task_context)
> > +		audit_log_task(ab);
> > +	audit_log_format(ab, "%sprog-id=%u event=%s",
> > +			 has_task_context ? " " : "",
> > +			 prog->aux->id, bpf_event_audit_str[event]);
> > +	audit_log_end(ab);
> 
> Single prog ID is enough for perf_event based framework to track everything
> about the programs and should be enough for audit.
> Could you please resend as proper patch with explicit 'From:' ?
> Since I'm not sure what is the proper authorship of the patch.. Daniel's or yours.

it's Daniel's I'll resend

jirka


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC] bpf: emit audit messages upon successful prog load and unload
  2019-11-20 14:38 [RFC] bpf: emit " Jiri Olsa
@ 2019-11-20 21:14 ` Alexei Starovoitov
  2019-11-20 21:30   ` Jiri Olsa
  0 siblings, 1 reply; 14+ messages in thread
From: Alexei Starovoitov @ 2019-11-20 21:14 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Jakub Kicinski,
	Steve Grubb, David Miller, Paul Moore, Eric Paris, Jiri Benc

On Wed, Nov 20, 2019 at 03:38:10PM +0100, Jiri Olsa wrote:
> 
> 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.

...

> +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_event event)
> +{
> +	bool has_task_context = event == BPF_EVENT_LOAD;
> +	struct audit_buffer *ab;
> +
> +	if (audit_enabled == AUDIT_OFF)
> +		return;
> +	ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
> +	if (unlikely(!ab))
> +		return;
> +	if (has_task_context)
> +		audit_log_task(ab);
> +	audit_log_format(ab, "%sprog-id=%u event=%s",
> +			 has_task_context ? " " : "",
> +			 prog->aux->id, bpf_event_audit_str[event]);
> +	audit_log_end(ab);

Single prog ID is enough for perf_event based framework to track everything
about the programs and should be enough for audit.
Could you please resend as proper patch with explicit 'From:' ?
Since I'm not sure what is the proper authorship of the patch.. Daniel's or yours.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] bpf: emit audit messages upon successful prog load and unload
@ 2019-11-20 14:38 Jiri Olsa
  2019-11-20 21:14 ` Alexei Starovoitov
  0 siblings, 1 reply; 14+ messages in thread
From: Jiri Olsa @ 2019-11-20 14:38 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann
  Cc: netdev, bpf, Andrii Nakryiko, Yonghong Song, Martin KaFai Lau,
	Jakub Kicinski, Steve Grubb, David Miller, Paul Moore,
	Eric Paris, Jiri Benc

Hi,
resending out the original audit support patch posted by Daniel
(rebased on current bpf-next/master code) for discussion. It was
declined in favor of perf based notification:
  https://marc.info/?l=linux-netdev&m=153866106418036&w=2

We tried to add perf based notification support to auditd,
but it did not fit and was nack-ed by audit guys:
  https://www.redhat.com/archives/linux-audit/2019-August/msg00004.html

Hopefully we could move forward with the initial change.

thanks/thoughts?
jirka


---
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 20 12:45:51 2019
  type=PROCTITLE msg=audit(1574271951.590:8974): proctitle="./test_verifier"
  type=SYSCALL msg=audit(1574271951.590:8974): arch=c000003e syscall=321 success=yes exit=14 a0=5 a1=7ffe2d923e80 a2=78 a3=0 items=0 ppid=742 pid=949 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=2 comm="test_verifier" exe="/root/bpf-next/tools/testing/selftests/bpf/test_verifier" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
  type=UNKNOWN[1334] msg=audit(1574271951.590:8974): auid=0 uid=0 gid=0 ses=2 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 pid=949 comm="test_verifier" exe="/root/bpf-next/tools/testing/selftests/bpf/test_verifier" prog-id=3260 event=LOAD
  ----
  time->Wed Nov 20 12:45:51 2019
type=UNKNOWN[1334] msg=audit(1574271951.590:8975): prog-id=3260 event=UNLOAD
  ----
  [...]

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 include/linux/audit.h      |  3 +++
 include/uapi/linux/audit.h |  1 +
 kernel/auditsc.c           |  2 +-
 kernel/bpf/syscall.c       | 31 +++++++++++++++++++++++++++++++
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index aee3dc9eb378..edd006f4597d 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -159,6 +159,7 @@ extern void		    audit_log_key(struct audit_buffer *ab,
 extern void		    audit_log_link_denied(const char *operation);
 extern void		    audit_log_lost(const char *message);
 
+extern void audit_log_task(struct audit_buffer *ab);
 extern int audit_log_task_context(struct audit_buffer *ab);
 extern void audit_log_task_info(struct audit_buffer *ab);
 
@@ -219,6 +220,8 @@ static inline void audit_log_key(struct audit_buffer *ab, char *key)
 { }
 static inline void audit_log_link_denied(const char *string)
 { }
+static inline void audit_log_task(struct audit_buffer *ab)
+{ }
 static inline int audit_log_task_context(struct audit_buffer *ab)
 {
 	return 0;
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/auditsc.c b/kernel/auditsc.c
index 4effe01ebbe2..9bf1045fedfa 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2545,7 +2545,7 @@ void __audit_ntp_log(const struct audit_ntp_data *ad)
 	audit_log_ntp_val(ad, "adjust",	AUDIT_NTP_ADJUST);
 }
 
-static void audit_log_task(struct audit_buffer *ab)
+void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
 	kgid_t gid;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index bac3becf9f90..17f4254495f2 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_PROG_ARRAY || \
@@ -1318,6 +1319,34 @@ static void free_used_maps(struct bpf_prog_aux *aux)
 	kfree(aux->used_maps);
 }
 
+enum bpf_event {
+	BPF_EVENT_LOAD,
+	BPF_EVENT_UNLOAD,
+};
+
+static const char * const bpf_event_audit_str[] = {
+	[BPF_EVENT_LOAD]   = "LOAD",
+	[BPF_EVENT_UNLOAD] = "UNLOAD",
+};
+
+static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_event event)
+{
+	bool has_task_context = event == BPF_EVENT_LOAD;
+	struct audit_buffer *ab;
+
+	if (audit_enabled == AUDIT_OFF)
+		return;
+	ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF);
+	if (unlikely(!ab))
+		return;
+	if (has_task_context)
+		audit_log_task(ab);
+	audit_log_format(ab, "%sprog-id=%u event=%s",
+			 has_task_context ? " " : "",
+			 prog->aux->id, bpf_event_audit_str[event]);
+	audit_log_end(ab);
+}
+
 int __bpf_prog_charge(struct user_struct *user, u32 pages)
 {
 	unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
@@ -1434,6 +1463,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_EVENT_UNLOAD);
 		/* bpf_prog_free_id() must be called first */
 		bpf_prog_free_id(prog, do_idr_lock);
 		__bpf_prog_put_noref(prog, true);
@@ -1843,6 +1873,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_EVENT_LOAD);
 
 	err = bpf_prog_new_fd(prog);
 	if (err < 0)
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-12-04 15:27 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28  9:16 [RFC] bpf: Emit audit messages upon successful prog load and unload Jiri Olsa
2019-11-28  9:18 ` Jiri Olsa
2019-12-02 23:00 ` Paul Moore
2019-12-03  4:57   ` Steve Grubb
2019-12-03  8:46     ` Jiri Olsa
2019-12-03  9:38   ` Jiri Olsa
2019-12-04  2:53     ` Paul Moore
2019-12-04 14:08       ` Jiri Olsa
2019-12-04 14:38         ` Paul Moore
2019-12-04 15:26           ` Jiri Olsa
2019-12-04 14:02   ` Jiri Olsa
  -- strict thread matches above, loose matches on Subject: below --
2019-11-20 14:38 [RFC] bpf: emit " Jiri Olsa
2019-11-20 21:14 ` Alexei Starovoitov
2019-11-20 21:30   ` Jiri Olsa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).