| Message ID | 20191120213816.8186-1-jolsa@kernel.org |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: emit audit messages upon successful prog load and unload | expand |
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!
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!
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
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?
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/
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).
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/
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
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 >
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); }
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); > }
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
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.
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
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
diff --git a/include/linux/audit.h b/include/linux/audit.h index aee3dc9eb378..edd006f4597d 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -159,6 +159,7 @@ extern void audit_log_key(struct audit_buffer *ab, extern void audit_log_link_denied(const char *operation); extern void audit_log_lost(const char *message); +extern void audit_log_task(struct audit_buffer *ab); extern int audit_log_task_context(struct audit_buffer *ab); extern void audit_log_task_info(struct audit_buffer *ab); @@ -219,6 +220,8 @@ static inline void audit_log_key(struct audit_buffer *ab, char *key) { } static inline void audit_log_link_denied(const char *string) { } +static inline void audit_log_task(struct audit_buffer *ab) +{ } static inline int audit_log_task_context(struct audit_buffer *ab) { return 0; diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index c89c6495983d..32a5db900f47 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -116,6 +116,7 @@ #define AUDIT_FANOTIFY 1331 /* Fanotify access decision */ #define AUDIT_TIME_INJOFFSET 1332 /* Timekeeping offset injected */ #define AUDIT_TIME_ADJNTPVAL 1333 /* NTP value adjustment */ +#define AUDIT_BPF 1334 /* BPF subsystem */ #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 4effe01ebbe2..9bf1045fedfa 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2545,7 +2545,7 @@ void __audit_ntp_log(const struct audit_ntp_data *ad) audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST); } -static void audit_log_task(struct audit_buffer *ab) +void audit_log_task(struct audit_buffer *ab) { kuid_t auid, uid; kgid_t gid; diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index bac3becf9f90..17f4254495f2 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -23,6 +23,7 @@ #include <linux/timekeeping.h> #include <linux/ctype.h> #include <linux/nospec.h> +#include <linux/audit.h> #include <uapi/linux/btf.h> #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PROG_ARRAY || \ @@ -1318,6 +1319,34 @@ static void free_used_maps(struct bpf_prog_aux *aux) kfree(aux->used_maps); } +enum bpf_event { + BPF_EVENT_LOAD, + BPF_EVENT_UNLOAD, +}; + +static const char * const bpf_event_audit_str[] = { + [BPF_EVENT_LOAD] = "LOAD", + [BPF_EVENT_UNLOAD] = "UNLOAD", +}; + +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_event event) +{ + bool has_task_context = event == BPF_EVENT_LOAD; + struct audit_buffer *ab; + + if (audit_enabled == AUDIT_OFF) + return; + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); + if (unlikely(!ab)) + return; + if (has_task_context) + audit_log_task(ab); + audit_log_format(ab, "%sprog-id=%u event=%s", + has_task_context ? " " : "", + prog->aux->id, bpf_event_audit_str[event]); + audit_log_end(ab); +} + int __bpf_prog_charge(struct user_struct *user, u32 pages) { unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; @@ -1434,6 +1463,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) { if (atomic64_dec_and_test(&prog->aux->refcnt)) { perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0); + bpf_audit_prog(prog, BPF_EVENT_UNLOAD); /* bpf_prog_free_id() must be called first */ bpf_prog_free_id(prog, do_idr_lock); __bpf_prog_put_noref(prog, true); @@ -1843,6 +1873,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr) */ bpf_prog_kallsyms_add(prog); perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0); + bpf_audit_prog(prog, BPF_EVENT_LOAD); err = bpf_prog_new_fd(prog); if (err < 0)