Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Paul Moore]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Alexei Starovoitov]

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?

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Paul Moore]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Jiri Olsa]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Paul Moore]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Jiri Olsa]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Jakub Kicinski]

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.

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Jiri Olsa]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Steve Grubb]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Daniel Borkmann]

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/

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Paul Moore]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Jiri Olsa]

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

Re: [PATCH] bpf: emit audit messages upon successful prog load and unload

[Jiri Olsa]

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
>