netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] bpf: emit audit messages upon successful prog load and unload
@ 2019-11-20 21:38 Jiri Olsa
  2019-11-20 21:46 ` Daniel Borkmann
  0 siblings, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2019-11-20 21: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

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 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] 16+ messages in thread

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

On 11/20/19 10:38 PM, 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 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>

LGTM, thanks for the rebase!

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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-20 21:46 ` Daniel Borkmann
@ 2019-11-20 21:48   ` Alexei Starovoitov
  2019-11-21 23:41     ` Paul Moore
  0 siblings, 1 reply; 16+ messages in thread
From: Alexei Starovoitov @ 2019-11-20 21:48 UTC (permalink / raw)
  To: Daniel Borkmann
  Cc: Jiri Olsa, Alexei Starovoitov, Network Development, 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 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 11/20/19 10:38 PM, 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 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>
>
> LGTM, thanks for the rebase!

Applied to bpf-next. Thanks!

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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-20 21:48   ` Alexei Starovoitov
@ 2019-11-21 23:41     ` Paul Moore
  2019-11-22  0:22       ` Alexei Starovoitov
                         ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Paul Moore @ 2019-11-21 23:41 UTC (permalink / raw)
  To: Alexei Starovoitov, linux-audit, Jiri Olsa, Daniel Borkmann
  Cc: Alexei Starovoitov, Network Development, bpf, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jakub Kicinski, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > On 11/20/19 10:38 PM, 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 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>
> >
> > LGTM, thanks for the rebase!
>
> Applied to bpf-next. Thanks!

[NOTE: added linux-audit to the To/CC line]

Wait a minute, why was the linux-audit list not CC'd on this?  Why are
you merging a patch into -next that adds to the uapi definition *and*
creates a new audit record while we are at -rc8?

Aside from that I'm concerned that you are relying on audit userspace
changes that might not be okay; I see the PR below, but I don't see
any comment on it from Steve (it is his audit userspace).  I also
don't see a corresponding test added to the audit-testsuite, which is
a common requirement for new audit functionality (link below).  I'm
also fairly certain we don't want this new BPF record to look like how
you've coded it up in bpf_audit_prog(); duplicating the fields with
audit_log_task() is wrong, you've either already got them via an
associated record (which you get from passing non-NULL as the first
parameter to audit_log_start()), or you don't because there is no
associated syscall/task (which you get from passing NULL as the first
parameter).  Please revert, un-merge, etc. this patch from bpf-next;
it should not go into Linus' tree as written.

Audit userspace PR:
* https://github.com/linux-audit/audit-userspace/pull/104

Audit test suite:
* https://github.com/linux-audit/audit-testsuite

Audit folks, here is a link to the thread in the archives:
* https://lore.kernel.org/bpf/20191120213816.8186-1-jolsa@kernel.org/T/#u

-- 
paul moore
www.paul-moore.com

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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-21 23:41     ` Paul Moore
@ 2019-11-22  0:22       ` Alexei Starovoitov
  2019-11-22  0:36         ` Paul Moore
  2019-11-22  0:25       ` Daniel Borkmann
                         ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Alexei Starovoitov @ 2019-11-22  0:22 UTC (permalink / raw)
  To: Paul Moore
  Cc: linux-audit, Jiri Olsa, Daniel Borkmann, Alexei Starovoitov,
	Network Development, bpf, Andrii Nakryiko, Yonghong Song,
	Martin KaFai Lau, Jakub Kicinski, Steve Grubb, David Miller,
	Eric Paris, Jiri Benc

On Thu, Nov 21, 2019 at 06:41:31PM -0500, Paul Moore wrote:
> On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > On 11/20/19 10:38 PM, 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 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>
> > >
> > > LGTM, thanks for the rebase!
> >
> > Applied to bpf-next. Thanks!
> 
> [NOTE: added linux-audit to the To/CC line]
> 
> Wait a minute, why was the linux-audit list not CC'd on this?  Why are
> you merging a patch into -next that adds to the uapi definition *and*
> creates a new audit record while we are at -rc8?
> 
> Aside from that I'm concerned that you are relying on audit userspace
> changes that might not be okay; I see the PR below, but I don't see
> any comment on it from Steve (it is his audit userspace).  I also
> don't see a corresponding test added to the audit-testsuite, which is
> a common requirement for new audit functionality (link below).  I'm
> also fairly certain we don't want this new BPF record to look like how
> you've coded it up in bpf_audit_prog(); duplicating the fields with
> audit_log_task() is wrong, you've either already got them via an
> associated record (which you get from passing non-NULL as the first
> parameter to audit_log_start()), or you don't because there is no
> associated syscall/task (which you get from passing NULL as the first
> parameter).  Please revert, un-merge, etc. this patch from bpf-next;
> it should not go into Linus' tree as written.

Sorry I didn't realize there was a disagreement.

Dave, could you please revert it in net-next?

> Audit userspace PR:
> * https://github.com/linux-audit/audit-userspace/pull/104

This PR does not use this new audit. It's doing everything via existing
perf_event notification. My understanding of Jiri's email was that netlink
style is preferred vs perf_event. Did I get it wrong?


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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-21 23:41     ` Paul Moore
  2019-11-22  0:22       ` Alexei Starovoitov
@ 2019-11-22  0:25       ` Daniel Borkmann
  2019-11-22  0:42         ` Paul Moore
  2019-11-22  9:32       ` Jiri Olsa
  2019-11-22  9:35       ` Jiri Olsa
  3 siblings, 1 reply; 16+ messages in thread
From: Daniel Borkmann @ 2019-11-22  0:25 UTC (permalink / raw)
  To: Paul Moore, Alexei Starovoitov, linux-audit, Jiri Olsa
  Cc: Alexei Starovoitov, Network Development, bpf, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jakub Kicinski, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On 11/22/19 12:41 AM, Paul Moore wrote:
> On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 11/20/19 10:38 PM, 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 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>
>>>
>>> LGTM, thanks for the rebase!
>>
>> Applied to bpf-next. Thanks!
> 
> [NOTE: added linux-audit to the To/CC line]
> 
> Wait a minute, why was the linux-audit list not CC'd on this?  Why are
> you merging a patch into -next that adds to the uapi definition *and*
> creates a new audit record while we are at -rc8?
> 
> Aside from that I'm concerned that you are relying on audit userspace
> changes that might not be okay; I see the PR below, but I don't see
> any comment on it from Steve (it is his audit userspace).  I also
> don't see a corresponding test added to the audit-testsuite, which is
> a common requirement for new audit functionality (link below).  I'm
> also fairly certain we don't want this new BPF record to look like how
> you've coded it up in bpf_audit_prog(); duplicating the fields with
> audit_log_task() is wrong, you've either already got them via an
> associated record (which you get from passing non-NULL as the first
> parameter to audit_log_start()), or you don't because there is no
> associated syscall/task (which you get from passing NULL as the first
> parameter).  Please revert, un-merge, etc. this patch from bpf-next;
> it should not go into Linus' tree as written.

Fair enough, up to you guys. My impression was that this is mainly coming
from RHEL use case [0] and given that the original patch was back in Oct
2018 [1] that you've sorted it out by now RH internally and agreed to proceed
with this patch for BPF given the rebase + resend ... seems not then. :(

The audit-userspace PR below is sitting there since August this year but
its for the perf event based approach and my understanding from Plumbers
conf was that the internal discussion was that a native integration was
needed hence the proposed resend now.

Given the change is mostly trivial, are there any major objections for Jiri
to follow-up? Otherwise worst case probably easier to revert in net-next.

> Audit userspace PR:
> * https://github.com/linux-audit/audit-userspace/pull/104
> 
> Audit test suite:
> * https://github.com/linux-audit/audit-testsuite
> 
> Audit folks, here is a link to the thread in the archives:
> * https://lore.kernel.org/bpf/20191120213816.8186-1-jolsa@kernel.org/T/#u

Thanks,
Daniel

   [0] slide 11, https://linuxplumbersconf.org/event/4/contributions/460/attachments/244/426/xdp-distro-view.pdf
   [1] https://lore.kernel.org/netdev/20181004135038.2876-1-daniel@iogearbox.net/

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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-22  0:22       ` Alexei Starovoitov
@ 2019-11-22  0:36         ` Paul Moore
  2019-11-22 19:23           ` Jiri Olsa
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2019-11-22  0:36 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: linux-audit, Jiri Olsa, Daniel Borkmann, Alexei Starovoitov,
	Network Development, bpf, Andrii Nakryiko, Yonghong Song,
	Martin KaFai Lau, Jakub Kicinski, Steve Grubb, David Miller,
	Eric Paris, Jiri Benc

On Thu, Nov 21, 2019 at 7:23 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Nov 21, 2019 at 06:41:31PM -0500, Paul Moore wrote:
> > On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > > On 11/20/19 10:38 PM, 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 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>
> > > >
> > > > LGTM, thanks for the rebase!
> > >
> > > Applied to bpf-next. Thanks!
> >
> > [NOTE: added linux-audit to the To/CC line]
> >
> > Wait a minute, why was the linux-audit list not CC'd on this?  Why are
> > you merging a patch into -next that adds to the uapi definition *and*
> > creates a new audit record while we are at -rc8?
> >
> > Aside from that I'm concerned that you are relying on audit userspace
> > changes that might not be okay; I see the PR below, but I don't see
> > any comment on it from Steve (it is his audit userspace).  I also
> > don't see a corresponding test added to the audit-testsuite, which is
> > a common requirement for new audit functionality (link below).  I'm
> > also fairly certain we don't want this new BPF record to look like how
> > you've coded it up in bpf_audit_prog(); duplicating the fields with
> > audit_log_task() is wrong, you've either already got them via an
> > associated record (which you get from passing non-NULL as the first
> > parameter to audit_log_start()), or you don't because there is no
> > associated syscall/task (which you get from passing NULL as the first
> > parameter).  Please revert, un-merge, etc. this patch from bpf-next;
> > it should not go into Linus' tree as written.
>
> Sorry I didn't realize there was a disagreement.
>
> Dave, could you please revert it in net-next?
>
> > Audit userspace PR:
> > * https://github.com/linux-audit/audit-userspace/pull/104
>
> This PR does not use this new audit. It's doing everything via existing
> perf_event notification. My understanding of Jiri's email was that netlink
> style is preferred vs perf_event. Did I get it wrong?

Perhaps confusion on my part regarding the audit-userspace PR.  The
commit description mentioned the audit userspace (the daemon most
likely) fetching additional info about the BPF program and this was
the only outstanding audit-userspace PR that had any mention of BPF.

However, getting back to netlink vs perf_event, if you want to
generate an audit record, it should happen via the audit subsystem
(and go up to the audit daemon via netlink).

-- 
paul moore
www.paul-moore.com

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

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

On Thu, Nov 21, 2019 at 7:25 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 11/22/19 12:41 AM, Paul Moore wrote:
> > On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >> On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>> On 11/20/19 10:38 PM, 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 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>
> >>>
> >>> LGTM, thanks for the rebase!
> >>
> >> Applied to bpf-next. Thanks!
> >
> > [NOTE: added linux-audit to the To/CC line]
> >
> > Wait a minute, why was the linux-audit list not CC'd on this?  Why are
> > you merging a patch into -next that adds to the uapi definition *and*
> > creates a new audit record while we are at -rc8?
> >
> > Aside from that I'm concerned that you are relying on audit userspace
> > changes that might not be okay; I see the PR below, but I don't see
> > any comment on it from Steve (it is his audit userspace).  I also
> > don't see a corresponding test added to the audit-testsuite, which is
> > a common requirement for new audit functionality (link below).  I'm
> > also fairly certain we don't want this new BPF record to look like how
> > you've coded it up in bpf_audit_prog(); duplicating the fields with
> > audit_log_task() is wrong, you've either already got them via an
> > associated record (which you get from passing non-NULL as the first
> > parameter to audit_log_start()), or you don't because there is no
> > associated syscall/task (which you get from passing NULL as the first
> > parameter).  Please revert, un-merge, etc. this patch from bpf-next;
> > it should not go into Linus' tree as written.
>
> Fair enough, up to you guys. My impression was that this is mainly coming
> from RHEL use case [0] and given that the original patch was back in Oct
> 2018 [1] that you've sorted it out by now RH internally and agreed to proceed
> with this patch for BPF given the rebase + resend ... seems not then. :(

For the record, I am not currently employed by RH and thus not part of
any RH internal discussions.  Although, even when I was, I would still
bristle at the idea of audit patches going in without CC'ing the audit
list and getting an ACK from the audit folks.  Internal discussions
within a company are fine, but the final discussion and debate should
happen on the public list.

> Given the change is mostly trivial, are there any major objections for Jiri
> to follow-up? Otherwise worst case probably easier to revert in net-next.

See my previous response for more info.  However, for starters the use
of audit_log_task() looks like the wrong thing to do here.  I also
want to see a test for our test suite so we can catch when someone
invariably breaks this in future and fix it.

>    [0] slide 11, https://linuxplumbersconf.org/event/4/contributions/460/attachments/244/426/xdp-distro-view.pdf
>    [1] https://lore.kernel.org/netdev/20181004135038.2876-1-daniel@iogearbox.net/

-- 
paul moore
www.paul-moore.com

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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-21 23:41     ` Paul Moore
  2019-11-22  0:22       ` Alexei Starovoitov
  2019-11-22  0:25       ` Daniel Borkmann
@ 2019-11-22  9:32       ` Jiri Olsa
  2019-11-22  9:35       ` Jiri Olsa
  3 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2019-11-22  9:32 UTC (permalink / raw)
  To: Paul Moore
  Cc: Alexei Starovoitov, linux-audit, Jiri Olsa, Daniel Borkmann,
	Alexei Starovoitov, Network Development, bpf, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jakub Kicinski, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On Thu, Nov 21, 2019 at 06:41:31PM -0500, Paul Moore wrote:
> On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > On 11/20/19 10:38 PM, 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 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>
> > >
> > > LGTM, thanks for the rebase!
> >
> > Applied to bpf-next. Thanks!
> 
> [NOTE: added linux-audit to the To/CC line]
> 
> Wait a minute, why was the linux-audit list not CC'd on this?  Why are
> you merging a patch into -next that adds to the uapi definition *and*
> creates a new audit record while we are at -rc8?

my bad sorry, I included only maintainers
there was previous RFC post:
  https://lore.kernel.org/netdev/20191120143810.8852-1-jolsa@kernel.org/

but I guess the patch followed up too fast

> Aside from that I'm concerned that you are relying on audit userspace
> changes that might not be okay; I see the PR below, but I don't see
> any comment on it from Steve (it is his audit userspace).  I also
> don't see a corresponding test added to the audit-testsuite, which is
> a common requirement for new audit functionality (link below).  I'm
> also fairly certain we don't want this new BPF record to look like how
> you've coded it up in bpf_audit_prog(); duplicating the fields with
> audit_log_task() is wrong, you've either already got them via an
> associated record (which you get from passing non-NULL as the first
> parameter to audit_log_start()), or you don't because there is no
> associated syscall/task (which you get from passing NULL as the first
> parameter).  Please revert, un-merge, etc. this patch from bpf-next;
> it should not go into Linus' tree as written.

the original audit approach for BPF notification was declined
in favor of perf-based approach:
  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

so we returned to the original approach

> 
> Audit userspace PR:
> * https://github.com/linux-audit/audit-userspace/pull/104

this is the perf-based notification approach, that got nacked

> 
> Audit test suite:
> * https://github.com/linux-audit/audit-testsuite

I'll check on these

thanks,
jirka


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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-21 23:41     ` Paul Moore
                         ` (2 preceding siblings ...)
  2019-11-22  9:32       ` Jiri Olsa
@ 2019-11-22  9:35       ` Jiri Olsa
  3 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2019-11-22  9:35 UTC (permalink / raw)
  To: Paul Moore
  Cc: Alexei Starovoitov, linux-audit, Jiri Olsa, Daniel Borkmann,
	Alexei Starovoitov, Network Development, bpf, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jakub Kicinski, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On Thu, Nov 21, 2019 at 06:41:31PM -0500, Paul Moore wrote:

SNIP

> a common requirement for new audit functionality (link below).  I'm
> also fairly certain we don't want this new BPF record to look like how
> you've coded it up in bpf_audit_prog(); duplicating the fields with
> audit_log_task() is wrong, you've either already got them via an
> associated record (which you get from passing non-NULL as the first
> parameter to audit_log_start()), or you don't because there is no
> associated syscall/task (which you get from passing NULL as the first

ok, I'll send change that reflects this.. together with the test

thanks,
jirka

> parameter).  Please revert, un-merge, etc. this patch from bpf-next;
> it should not go into Linus' tree as written.
> 
> Audit userspace PR:
> * https://github.com/linux-audit/audit-userspace/pull/104
> 
> Audit test suite:
> * https://github.com/linux-audit/audit-testsuite
> 
> Audit folks, here is a link to the thread in the archives:
> * https://lore.kernel.org/bpf/20191120213816.8186-1-jolsa@kernel.org/T/#u
> 
> -- 
> paul moore
> www.paul-moore.com
> 


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

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

On Thu, Nov 21, 2019 at 07:36:29PM -0500, Paul Moore wrote:
> On Thu, Nov 21, 2019 at 7:23 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> > On Thu, Nov 21, 2019 at 06:41:31PM -0500, Paul Moore wrote:
> > > On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov
> > > <alexei.starovoitov@gmail.com> wrote:
> > > > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > > > On 11/20/19 10:38 PM, 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 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>
> > > > >
> > > > > LGTM, thanks for the rebase!
> > > >
> > > > Applied to bpf-next. Thanks!
> > >
> > > [NOTE: added linux-audit to the To/CC line]
> > >
> > > Wait a minute, why was the linux-audit list not CC'd on this?  Why are
> > > you merging a patch into -next that adds to the uapi definition *and*
> > > creates a new audit record while we are at -rc8?
> > >
> > > Aside from that I'm concerned that you are relying on audit userspace
> > > changes that might not be okay; I see the PR below, but I don't see
> > > any comment on it from Steve (it is his audit userspace).  I also
> > > don't see a corresponding test added to the audit-testsuite, which is
> > > a common requirement for new audit functionality (link below).  I'm
> > > also fairly certain we don't want this new BPF record to look like how
> > > you've coded it up in bpf_audit_prog(); duplicating the fields with
> > > audit_log_task() is wrong, you've either already got them via an
> > > associated record (which you get from passing non-NULL as the first
> > > parameter to audit_log_start()), or you don't because there is no
> > > associated syscall/task (which you get from passing NULL as the first
> > > parameter).  Please revert, un-merge, etc. this patch from bpf-next;
> > > it should not go into Linus' tree as written.
> >
> > Sorry I didn't realize there was a disagreement.
> >
> > Dave, could you please revert it in net-next?
> >
> > > Audit userspace PR:
> > > * https://github.com/linux-audit/audit-userspace/pull/104
> >
> > This PR does not use this new audit. It's doing everything via existing
> > perf_event notification. My understanding of Jiri's email was that netlink
> > style is preferred vs perf_event. Did I get it wrong?
> 
> Perhaps confusion on my part regarding the audit-userspace PR.  The
> commit description mentioned the audit userspace (the daemon most
> likely) fetching additional info about the BPF program and this was
> the only outstanding audit-userspace PR that had any mention of BPF.
> 
> However, getting back to netlink vs perf_event, if you want to
> generate an audit record, it should happen via the audit subsystem
> (and go up to the audit daemon via netlink).

Paul,
would following output be ok:

    type=SYSCALL msg=audit(1574445211.897:28015): arch=c000003e syscall=321 success=no exit=-13 a0=5 a1=7fff09ac6c60 a2=78 a3=6 items=0 ppid=1408 pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="test_verifier" exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
    type=PROCTITLE msg=audit(1574445211.897:28015): proctitle="./test_verifier"
    type=BPF msg=audit(1574445211.897:28016): prog-id=8103 event=LOAD

    type=SYSCALL msg=audit(1574445211.897:28016): arch=c000003e syscall=321 success=yes exit=14 a0=5 a1=7fff09ac6b80 a2=78 a3=0 items=0 ppid=1408 pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="test_verifier" exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
    type=PROCTITLE msg=audit(1574445211.897:28016): proctitle="./test_verifier"
    type=BPF msg=audit(1574445211.897:28017): prog-id=8103 event=UNLOAD

I assume for audit-userspace and audit-testsuite the change will
go in as github PR, right? I have the auditd change ready and will
add test shortly.

thanks,
jirka


---
 include/linux/audit.h | 4 ----
 kernel/auditsc.c      | 2 +-
 kernel/bpf/syscall.c  | 6 +-----
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 18925d924c73..c69d2776d197 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -358,8 +358,6 @@ static inline void audit_ptrace(struct task_struct *t)
 		__audit_ptrace(t);
 }
 
-extern void audit_log_task(struct audit_buffer *ab);
-
 				/* Private API (for audit.c only) */
 extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
 extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
@@ -648,8 +646,6 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
 static inline void audit_ptrace(struct task_struct *t)
 { }
 
-static inline void audit_log_task(struct audit_buffer *ab)
-{ }
 #define audit_n_rules 0
 #define audit_signals 0
 #endif /* CONFIG_AUDITSYSCALL */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 9bf1045fedfa..4effe01ebbe2 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);
 }
 
-void audit_log_task(struct audit_buffer *ab)
+static 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 b51ecb9644d0..e3a7fa4d7a82 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1334,7 +1334,6 @@ static const char * const bpf_event_audit_str[] = {
 
 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)
@@ -1342,10 +1341,7 @@ static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_event event)
 	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 ? " " : "",
+	audit_log_format(ab, "prog-id=%u event=%s",
 			 prog->aux->id, bpf_event_audit_str[event]);
 	audit_log_end(ab);
 }
-- 
2.23.0


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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-22 19:23           ` Jiri Olsa
@ 2019-11-22 21:19             ` Paul Moore
  2019-11-23  8:57               ` Jiri Olsa
  2019-11-25 18:38               ` Steve Grubb
  0 siblings, 2 replies; 16+ messages in thread
From: Paul Moore @ 2019-11-22 21:19 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Alexei Starovoitov, linux-audit, Jiri Olsa, Daniel Borkmann,
	Alexei Starovoitov, Network Development, bpf, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jakub Kicinski, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On Fri, Nov 22, 2019 at 2:24 PM Jiri Olsa <jolsa@redhat.com> wrote:
> Paul,
> would following output be ok:
>
>     type=SYSCALL msg=audit(1574445211.897:28015): arch=c000003e syscall=321 success=no exit=-13 a0=5 a1=7fff09ac6c60 a2=78 a3=6 items=0 ppid=1408 pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="test_verifier" exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
>     type=PROCTITLE msg=audit(1574445211.897:28015): proctitle="./test_verifier"
>     type=BPF msg=audit(1574445211.897:28016): prog-id=8103 event=LOAD
>
>     type=SYSCALL msg=audit(1574445211.897:28016): arch=c000003e syscall=321 success=yes exit=14 a0=5 a1=7fff09ac6b80 a2=78 a3=0 items=0 ppid=1408 pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="test_verifier" exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
>     type=PROCTITLE msg=audit(1574445211.897:28016): proctitle="./test_verifier"
>     type=BPF msg=audit(1574445211.897:28017): prog-id=8103 event=UNLOAD

There is some precedence in using "op=" instead of "event=" (an audit
"event" is already a thing, using "event=" here might get confusing).
I suppose if we are getting really nit-picky you might want to
lower-case the LOAD/UNLOAD, but generally Steve cares more about these
things than I do.

For reference, we have a searchable database of fields here:
* https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv

> I assume for audit-userspace and audit-testsuite the change will
> go in as github PR, right? I have the auditd change ready and will
> add test shortly.

You can submit the audit-testsuite either as a GH PR or as a
patch(set) to the linux-audit mailing list, both work equally well.  I
believe has the same policy for his userspace tools, but I'll let him
speak for himself.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 18925d924c73..c69d2776d197 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -358,8 +358,6 @@ static inline void audit_ptrace(struct task_struct *t)
>                 __audit_ptrace(t);
>  }
>
> -extern void audit_log_task(struct audit_buffer *ab);
> -
>                                 /* Private API (for audit.c only) */
>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
> @@ -648,8 +646,6 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>
> -static inline void audit_log_task(struct audit_buffer *ab)
> -{ }
>  #define audit_n_rules 0
>  #define audit_signals 0
>  #endif /* CONFIG_AUDITSYSCALL */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9bf1045fedfa..4effe01ebbe2 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);
>  }
>
> -void audit_log_task(struct audit_buffer *ab)
> +static void audit_log_task(struct audit_buffer *ab)

I'm slightly concerned that this is based on top of your other patch
which was NACK'ed.  I might not have been clear before, but with the
merge window set to open in a few days, and this change affecting the
kernel interface (uapi, etc.) and lacking a test, this isn't something
that I see as a candidate for the upcoming merge window.  *Please*
revert your original patch first; if you think I'm cranky now I can
promise I'll be a lot more cranky if I see the original patch in -rc1
;)

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index b51ecb9644d0..e3a7fa4d7a82 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -1334,7 +1334,6 @@ static const char * const bpf_event_audit_str[] = {
>
>  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)
> @@ -1342,10 +1341,7 @@ static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_event event)
>         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 ? " " : "",
> +       audit_log_format(ab, "prog-id=%u event=%s",
>                          prog->aux->id, bpf_event_audit_str[event]);

Other than the "op" instead of "event", this looks reasonable to me.
I would give Steve a chance to comment on it from the userspace side
of things.

>         audit_log_end(ab);
>  }

-- 
paul moore
www.paul-moore.com

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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-22 21:19             ` Paul Moore
@ 2019-11-23  8:57               ` Jiri Olsa
  2019-11-23 18:03                 ` Jakub Kicinski
  2019-11-25 18:38               ` Steve Grubb
  1 sibling, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2019-11-23  8:57 UTC (permalink / raw)
  To: Paul Moore
  Cc: Alexei Starovoitov, linux-audit, Jiri Olsa, Daniel Borkmann,
	Alexei Starovoitov, Network Development, bpf, Andrii Nakryiko,
	Yonghong Song, Martin KaFai Lau, Jakub Kicinski, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On Fri, Nov 22, 2019 at 04:19:55PM -0500, Paul Moore wrote:
> On Fri, Nov 22, 2019 at 2:24 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > Paul,
> > would following output be ok:
> >
> >     type=SYSCALL msg=audit(1574445211.897:28015): arch=c000003e syscall=321 success=no exit=-13 a0=5 a1=7fff09ac6c60 a2=78 a3=6 items=0 ppid=1408 pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="test_verifier" exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
> >     type=PROCTITLE msg=audit(1574445211.897:28015): proctitle="./test_verifier"
> >     type=BPF msg=audit(1574445211.897:28016): prog-id=8103 event=LOAD
> >
> >     type=SYSCALL msg=audit(1574445211.897:28016): arch=c000003e syscall=321 success=yes exit=14 a0=5 a1=7fff09ac6b80 a2=78 a3=0 items=0 ppid=1408 pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=1 comm="test_verifier" exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
> >     type=PROCTITLE msg=audit(1574445211.897:28016): proctitle="./test_verifier"
> >     type=BPF msg=audit(1574445211.897:28017): prog-id=8103 event=UNLOAD
> 
> There is some precedence in using "op=" instead of "event=" (an audit
> "event" is already a thing, using "event=" here might get confusing).
> I suppose if we are getting really nit-picky you might want to
> lower-case the LOAD/UNLOAD, but generally Steve cares more about these
> things than I do.
> 
> For reference, we have a searchable database of fields here:
> * https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv

I'm fine with "op", Daniel, Alexei?

> 
> > I assume for audit-userspace and audit-testsuite the change will
> > go in as github PR, right? I have the auditd change ready and will
> > add test shortly.
> 
> You can submit the audit-testsuite either as a GH PR or as a
> patch(set) to the linux-audit mailing list, both work equally well.  I
> believe has the same policy for his userspace tools, but I'll let him
> speak for himself.

ok

> 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 18925d924c73..c69d2776d197 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -358,8 +358,6 @@ static inline void audit_ptrace(struct task_struct *t)
> >                 __audit_ptrace(t);
> >  }
> >
> > -extern void audit_log_task(struct audit_buffer *ab);
> > -
> >                                 /* Private API (for audit.c only) */
> >  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> >  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
> > @@ -648,8 +646,6 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
> >  static inline void audit_ptrace(struct task_struct *t)
> >  { }
> >
> > -static inline void audit_log_task(struct audit_buffer *ab)
> > -{ }
> >  #define audit_n_rules 0
> >  #define audit_signals 0
> >  #endif /* CONFIG_AUDITSYSCALL */
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 9bf1045fedfa..4effe01ebbe2 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);
> >  }
> >
> > -void audit_log_task(struct audit_buffer *ab)
> > +static void audit_log_task(struct audit_buffer *ab)
> 
> I'm slightly concerned that this is based on top of your other patch
> which was NACK'ed.  I might not have been clear before, but with the
> merge window set to open in a few days, and this change affecting the
> kernel interface (uapi, etc.) and lacking a test, this isn't something
> that I see as a candidate for the upcoming merge window.  *Please*
> revert your original patch first; if you think I'm cranky now I can
> promise I'll be a lot more cranky if I see the original patch in -rc1
> ;)

no worries, I'm used to cranky ;-)
Alexei already asked Dave to revert this in previous email,
so that should happen

thanks,
jirka


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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-23  8:57               ` Jiri Olsa
@ 2019-11-23 18:03                 ` Jakub Kicinski
  2019-11-24 22:38                   ` Jiri Olsa
  0 siblings, 1 reply; 16+ messages in thread
From: Jakub Kicinski @ 2019-11-23 18:03 UTC (permalink / raw)
  To: Jiri Olsa
  Cc: Paul Moore, Alexei Starovoitov, linux-audit, Jiri Olsa,
	Daniel Borkmann, Alexei Starovoitov, Network Development, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On Sat, 23 Nov 2019 09:57:19 +0100, Jiri Olsa wrote:
> Alexei already asked Dave to revert this in previous email,
> so that should happen

Reverted in net-next now.

But this is not really how this should work. You should post a proper
revert patch to netdev for review, with an explanation in the commit
message etc.

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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-23 18:03                 ` Jakub Kicinski
@ 2019-11-24 22:38                   ` Jiri Olsa
  0 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2019-11-24 22:38 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Paul Moore, Alexei Starovoitov, linux-audit, Jiri Olsa,
	Daniel Borkmann, Alexei Starovoitov, Network Development, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Steve Grubb,
	David Miller, Eric Paris, Jiri Benc

On Sat, Nov 23, 2019 at 10:03:40AM -0800, Jakub Kicinski wrote:
> On Sat, 23 Nov 2019 09:57:19 +0100, Jiri Olsa wrote:
> > Alexei already asked Dave to revert this in previous email,
> > so that should happen
> 
> Reverted in net-next now.
> 
> But this is not really how this should work. You should post a proper
> revert patch to netdev for review, with an explanation in the commit
> message etc.

I had no idea I need to post the revert, sorry
will do next time

thanks,
jirka


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

* Re: [PATCH] bpf: emit audit messages upon successful prog load and unload
  2019-11-22 21:19             ` Paul Moore
  2019-11-23  8:57               ` Jiri Olsa
@ 2019-11-25 18:38               ` Steve Grubb
  1 sibling, 0 replies; 16+ messages in thread
From: Steve Grubb @ 2019-11-25 18:38 UTC (permalink / raw)
  To: Paul Moore
  Cc: Jiri Olsa, Alexei Starovoitov, linux-audit, Jiri Olsa,
	Daniel Borkmann, Alexei Starovoitov, Network Development, bpf,
	Andrii Nakryiko, Yonghong Song, Martin KaFai Lau, Jakub Kicinski,
	David Miller, Eric Paris, Jiri Benc

Hello,

On Friday, November 22, 2019 4:19:55 PM EST Paul Moore wrote:
> On Fri, Nov 22, 2019 at 2:24 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > Paul,
> > would following output be ok:
> > 
> > type=SYSCALL msg=audit(1574445211.897:28015): arch=c000003e syscall=321
> > success=no exit=-13 a0=5 a1=7fff09ac6c60 a2=78 a3=6 items=0 ppid=1408
> > pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> > fsgid=0 tty=pts0 ses=1 comm="test_verifier"
> > exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier"
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root"
> > EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root"
> > FSGID="root" type=PROCTITLE msg=audit(1574445211.897:28015):
> > proctitle="./test_verifier" type=BPF msg=audit(1574445211.897:28016):
> > prog-id=8103 event=LOAD
> > 
> > type=SYSCALL msg=audit(1574445211.897:28016): arch=c000003e syscall=321
> > success=yes exit=14 a0=5 a1=7fff09ac6b80 a2=78 a3=0 items=0 ppid=1408
> > pid=9266 auid=1001 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> > fsgid=0 tty=pts0 ses=1 comm="test_verifier"
> > exe="/home/jolsa/linux/tools/testing/selftests/bpf/test_verifier"
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
> > key=(null)ARCH=x86_64 SYSCALL=bpf AUID="jolsa" UID="root" GID="root"
> > EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root"
> > FSGID="root" type=PROCTITLE msg=audit(1574445211.897:28016):
> > proctitle="./test_verifier" type=BPF msg=audit(1574445211.897:28017):
> > prog-id=8103 event=UNLOAD
>
> There is some precedence in using "op=" instead of "event=" (an audit
> "event" is already a thing, using "event=" here might get confusing).
> I suppose if we are getting really nit-picky you might want to
> lower-case the LOAD/UNLOAD, but generally Steve cares more about these
> things than I do.
> 
> For reference, we have a searchable database of fields here:
> *
> https://github.com/linux-audit/audit-documentation/blob/master/specs/field
> s/field-dictionary.csv

Paul's comments are correct. We generally use op for what operation is being 
performed. This approach looks better. This is fitting in with the audit way 
of doing things. I don't think there would be any user space issues adding 
support for the BPF record.

-Steve



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

end of thread, other threads:[~2019-11-25 18:38 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 21:38 [PATCH] bpf: emit audit messages upon successful prog load and unload Jiri Olsa
2019-11-20 21:46 ` Daniel Borkmann
2019-11-20 21:48   ` Alexei Starovoitov
2019-11-21 23:41     ` Paul Moore
2019-11-22  0:22       ` Alexei Starovoitov
2019-11-22  0:36         ` Paul Moore
2019-11-22 19:23           ` Jiri Olsa
2019-11-22 21:19             ` Paul Moore
2019-11-23  8:57               ` Jiri Olsa
2019-11-23 18:03                 ` Jakub Kicinski
2019-11-24 22:38                   ` Jiri Olsa
2019-11-25 18:38               ` Steve Grubb
2019-11-22  0:25       ` Daniel Borkmann
2019-11-22  0:42         ` Paul Moore
2019-11-22  9:32       ` Jiri Olsa
2019-11-22  9:35       ` 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).