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

older

Security audit rules

[PATCH AUTOSEL 4.9 04/13]...

Paul Moore

Thursday, 21 November 2019 Thu, 21 Nov '19

5:41 p.m.

On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov <alexei.starovoitov(a)gmail.com> wrote:

...

On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel(a)iogearbox.net> wrote: > On 11/20/19 10:38 PM, Jiri Olsa wrote: > > From: Daniel Borkmann <daniel(a)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(a)iogearbox.net> > > Signed-off-by: Jiri Olsa <jolsa(a)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

Show replies by date

Alexei Starovoitov

Thursday, 21 November Thu, 21 Nov

6:22 p.m.

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

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(a)gmail.com> wrote: > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel(a)iogearbox.net> wrote: > > On 11/20/19 10:38 PM, Jiri Olsa wrote: > > > From: Daniel Borkmann <daniel(a)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(a)iogearbox.net> > > > Signed-off-by: Jiri Olsa <jolsa(a)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?

Paul Moore

6:36 p.m.

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

On Thu, Nov 21, 2019 at 7:23 PM Alexei Starovoitov <alexei.starovoitov(a)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(a)gmail.com> wrote: > > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel(a)iogearbox.net> wrote: > > > On 11/20/19 10:38 PM, Jiri Olsa wrote: > > > > From: Daniel Borkmann <daniel(a)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(a)iogearbox.net> > > > > Signed-off-by: Jiri Olsa <jolsa(a)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

Jiri Olsa

Friday, 22 November Fri, 22 Nov

1:23 p.m.

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

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(a)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(a)gmail.com> wrote: > > > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel(a)iogearbox.net> wrote: > > > > On 11/20/19 10:38 PM, Jiri Olsa wrote: > > > > > From: Daniel Borkmann <daniel(a)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(a)iogearbox.net> > > > > > Signed-off-by: Jiri Olsa <jolsa(a)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

Paul Moore

3:19 p.m.

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

On Fri, Nov 22, 2019 at 2:24 PM Jiri Olsa <jolsa(a)redhat.com> wrote:

...

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

...

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

Jiri Olsa

Saturday, 23 November Sat, 23 Nov

2:57 a.m.

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

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(a)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/fiel...

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.

...

> 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

Jakub Kicinski

12:03 p.m.

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

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.

Jiri Olsa

Sunday, 24 November Sun, 24 Nov

4:38 p.m.

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

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

Steve Grubb

Monday, 25 November Mon, 25 Nov

12:38 p.m.

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

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(a)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

Daniel Borkmann

Thursday, 21 November Thu, 21 Nov

6:25 p.m.

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

On 11/22/19 12:41 AM, Paul Moore wrote:

...

On Wed, Nov 20, 2019 at 4:49 PM Alexei Starovoitov <alexei.starovoitov(a)gmail.com> wrote: > On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel(a)iogearbox.net> wrote: >> On 11/20/19 10:38 PM, Jiri Olsa wrote: >>> From: Daniel Borkmann <daniel(a)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(a)iogearbox.net> >>> Signed-off-by: Jiri Olsa <jolsa(a)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/4... [1] https://lore.kernel.org/netdev/20181004135038.2876-1-daniel@iogearbox.net/

Paul Moore

6:42 p.m.

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

On Thu, Nov 21, 2019 at 7:25 PM Daniel Borkmann <daniel(a)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(a)gmail.com> wrote: >> On Wed, Nov 20, 2019 at 1:46 PM Daniel Borkmann <daniel(a)iogearbox.net> wrote: >>> On 11/20/19 10:38 PM, Jiri Olsa wrote: >>>> From: Daniel Borkmann <daniel(a)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(a)iogearbox.net> >>>> Signed-off-by: Jiri Olsa <jolsa(a)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/4... [1] https://lore.kernel.org/netdev/20181004135038.2876-1-daniel@iogearbox.net/

-- paul moore www.paul-moore.com

Jiri Olsa

Friday, 22 November Fri, 22 Nov

3:32 a.m.

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

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

...

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

Jiri Olsa

3:35 a.m.

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

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

1854

days inactive

1858

days old

linux-audit@lists.linux-audit.osci.io

Manage subscription

12 comments

6 participants

tags (0)

participants (6)

Alexei Starovoitov
Daniel Borkmann
Jakub Kicinski
Jiri Olsa
Paul Moore
Steve Grubb

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

2004

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