| Message ID | 20191128091633.29275-1-jolsa@kernel.org |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [RFC] bpf: Emit audit messages upon successful prog load and unload | expand |
On Thu, Nov 28, 2019 at 10:16:32AM +0100, Jiri Olsa wrote: > From: Daniel Borkmann <daniel@iogearbox.net> > > Allow for audit messages to be emitted upon BPF program load and > unload for having a timeline of events. The load itself is in > syscall context, so additional info about the process initiating > the BPF prog creation can be logged and later directly correlated > to the unload event. > > The only info really needed from BPF side is the globally unique > prog ID where then audit user space tooling can query / dump all > info needed about the specific BPF program right upon load event > and enrich the record, thus these changes needed here can be kept > small and non-intrusive to the core. > > Raw example output: > > # auditctl -D > # auditctl -a always,exit -F arch=x86_64 -S bpf > # ausearch --start recent -m 1334 > ... > ---- > time->Wed Nov 27 16:04:13 2019 > type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf" > type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321 \ > success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477 \ > pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001 \ > egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf" \ > exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf" \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) > type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD > ---- > time->Wed Nov 27 16:04:13 2019 > type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 op=UNLOAD > ... > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Co-developed-by: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> fyi I prepared userspace changes: https://github.com/olsajiri/audit-userspace/commit/3108a81fa8d937f07b4c78be8ae00fcd3d64f94d https://github.com/olsajiri/audit-testsuite/commit/16888ea7f14fa0269feef623d2a96f15f9ea71c9 I'll sumbit github PRs once the kernel change is pulled in thanks, jirka
On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote: > From: Daniel Borkmann <daniel@iogearbox.net> > > Allow for audit messages to be emitted upon BPF program load and > unload for having a timeline of events. The load itself is in > syscall context, so additional info about the process initiating > the BPF prog creation can be logged and later directly correlated > to the unload event. > > The only info really needed from BPF side is the globally unique > prog ID where then audit user space tooling can query / dump all > info needed about the specific BPF program right upon load event > and enrich the record, thus these changes needed here can be kept > small and non-intrusive to the core. > > Raw example output: > > # auditctl -D > # auditctl -a always,exit -F arch=x86_64 -S bpf > # ausearch --start recent -m 1334 > ... > ---- > time->Wed Nov 27 16:04:13 2019 > type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf" > type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321 \ > success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477 \ > pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001 \ > egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf" \ > exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf" \ > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) > type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD > ---- > time->Wed Nov 27 16:04:13 2019 > type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 op=UNLOAD > ... > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Co-developed-by: Jiri Olsa <jolsa@kernel.org> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/uapi/linux/audit.h | 1 + > kernel/bpf/syscall.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 28 insertions(+) Hi all, sorry for the delay; the merge window in combination with the holiday in the US bumped this back a bit. Small comments inline below ... > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -23,6 +23,7 @@ > #include <linux/timekeeping.h> > #include <linux/ctype.h> > #include <linux/nospec.h> > +#include <linux/audit.h> > #include <uapi/linux/btf.h> > > #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ > @@ -1306,6 +1307,30 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog) > return 0; > } > > +enum bpf_audit { > + BPF_AUDIT_LOAD, > + BPF_AUDIT_UNLOAD, > +}; > + > +static const char * const bpf_audit_str[] = { > + [BPF_AUDIT_LOAD] = "LOAD", > + [BPF_AUDIT_UNLOAD] = "UNLOAD", > +}; > + > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > +{ > + struct audit_buffer *ab; > + > + if (audit_enabled == AUDIT_OFF) > + return; I think you would probably also want to check the results of audit_dummy_context() here as well, see all the various audit_XXX() functions in include/linux/audit.h as an example. You'll see a pattern similar to the following: static inline void audit_foo(...) { if (unlikely(!audit_dummy_context())) __audit_foo(...) } > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); > + if (unlikely(!ab)) > + return; > + audit_log_format(ab, "prog-id=%u op=%s", > + prog->aux->id, bpf_audit_str[op]); Is it worth putting some checks in here to make sure that you don't blow past the end of the bpf_audit_str array? > + audit_log_end(ab); > +} The audit record format looks much better now, thank you. Although I do wonder if you want bpf_audit_prog() to live in kernel/bpf/syscall.c or in kernel/auditsc.c? There is plenty of precedence for moving it into auditsc.c and defining a no-op version for when CONFIG_AUDITSYSCALL is not enabled, but I personally don't feel that strongly about either option. I just wanted to mention this in case you weren't already aware. If you do keep it in syscall.c, I don't think there is a need to implement a no-op version dependent on CONFIG_AUDITSYSCALL; that will just clutter the code. If you do move it to auditsc.c please change the name to audit_bpf()/__audit_bpf() so it matches the other functions; if you keep it in syscall.c you can name it whatever you like :) -- paul moore www.paul-moore.com
On Monday, December 2, 2019 6:00:14 PM EST Paul Moore wrote: > On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote: > > From: Daniel Borkmann <daniel@iogearbox.net> > > > > Allow for audit messages to be emitted upon BPF program load and > > unload for having a timeline of events. The load itself is in > > syscall context, so additional info about the process initiating > > the BPF prog creation can be logged and later directly correlated > > to the unload event. > > > > The only info really needed from BPF side is the globally unique > > prog ID where then audit user space tooling can query / dump all > > info needed about the specific BPF program right upon load event > > and enrich the record, thus these changes needed here can be kept > > small and non-intrusive to the core. > > > > Raw example output: > > # auditctl -D > > # auditctl -a always,exit -F arch=x86_64 -S bpf > > # ausearch --start recent -m 1334 > > ... > > ---- > > time->Wed Nov 27 16:04:13 2019 > > type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf" > > type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321 > > \> > > success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477 > > \ > > pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001 > > \ > > egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf" > > \ > > exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf" > > \ > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) > > > > type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD > > ---- > > time->Wed Nov 27 16:04:13 2019 > > type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 > > op=UNLOAD > > ... > > > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > > Co-developed-by: Jiri Olsa <jolsa@kernel.org> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > > > include/uapi/linux/audit.h | 1 + > > kernel/bpf/syscall.c | 27 +++++++++++++++++++++++++++ > > 2 files changed, 28 insertions(+) > > Hi all, sorry for the delay; the merge window in combination with the > holiday in the US bumped this back a bit. Small comments inline below > ... > > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -23,6 +23,7 @@ > > > > #include <linux/timekeeping.h> > > #include <linux/ctype.h> > > #include <linux/nospec.h> > > > > +#include <linux/audit.h> > > > > #include <uapi/linux/btf.h> > > > > #define IS_FD_ARRAY(map) ((map)->map_type == > > BPF_MAP_TYPE_PERF_EVENT_ARRAY || \> > > @@ -1306,6 +1307,30 @@ static int find_prog_type(enum bpf_prog_type type, > > struct bpf_prog *prog)> > > return 0; > > > > } > > > > +enum bpf_audit { > > + BPF_AUDIT_LOAD, > > + BPF_AUDIT_UNLOAD, > > +}; > > + > > +static const char * const bpf_audit_str[] = { > > + [BPF_AUDIT_LOAD] = "LOAD", > > + [BPF_AUDIT_UNLOAD] = "UNLOAD", > > +}; > > + > > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit > > op) +{ > > + struct audit_buffer *ab; > > + > > + if (audit_enabled == AUDIT_OFF) > > + return; > > I think you would probably also want to check the results of > audit_dummy_context() here as well, see all the various audit_XXX() > functions in include/linux/audit.h as an example. You'll see a > pattern similar to the following: > > static inline void audit_foo(...) > { > if (unlikely(!audit_dummy_context())) > __audit_foo(...) > } > > > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); > > + if (unlikely(!ab)) > > + return; > > + audit_log_format(ab, "prog-id=%u op=%s", > > + prog->aux->id, bpf_audit_str[op]); > > Is it worth putting some checks in here to make sure that you don't > blow past the end of the bpf_audit_str array? I am wondering if prog-id was really the only information that was needed? Is it meaningful to other tools? Does that correlate to anything in /proc? In earlier discussion, it sounded like more information was needed to be sure what was happening. -Steve > > + audit_log_end(ab); > > +} > > The audit record format looks much better now, thank you. Although I > do wonder if you want bpf_audit_prog() to live in kernel/bpf/syscall.c > or in kernel/auditsc.c? There is plenty of precedence for moving it > into auditsc.c and defining a no-op version for when > CONFIG_AUDITSYSCALL is not enabled, but I personally don't feel that > strongly about either option. I just wanted to mention this in case > you weren't already aware. > > If you do keep it in syscall.c, I don't think there is a need to > implement a no-op version dependent on CONFIG_AUDITSYSCALL; that will > just clutter the code. > > If you do move it to auditsc.c please change the name to > audit_bpf()/__audit_bpf() so it matches the other functions; if you > keep it in syscall.c you can name it whatever you like :) > > -- > paul moore > www.paul-moore.com
On Mon, Dec 02, 2019 at 11:57:22PM -0500, Steve Grubb wrote: > On Monday, December 2, 2019 6:00:14 PM EST Paul Moore wrote: > > On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote: > > > From: Daniel Borkmann <daniel@iogearbox.net> > > > > > > Allow for audit messages to be emitted upon BPF program load and > > > unload for having a timeline of events. The load itself is in > > > syscall context, so additional info about the process initiating > > > the BPF prog creation can be logged and later directly correlated > > > to the unload event. > > > > > > The only info really needed from BPF side is the globally unique > > > prog ID where then audit user space tooling can query / dump all > > > info needed about the specific BPF program right upon load event > > > and enrich the record, thus these changes needed here can be kept > > > small and non-intrusive to the core. SNIP > > I think you would probably also want to check the results of > > audit_dummy_context() here as well, see all the various audit_XXX() > > functions in include/linux/audit.h as an example. You'll see a > > pattern similar to the following: > > > > static inline void audit_foo(...) > > { > > if (unlikely(!audit_dummy_context())) > > __audit_foo(...) > > } > > > > > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); > > > + if (unlikely(!ab)) > > > + return; > > > + audit_log_format(ab, "prog-id=%u op=%s", > > > + prog->aux->id, bpf_audit_str[op]); > > > > Is it worth putting some checks in here to make sure that you don't > > blow past the end of the bpf_audit_str array? > > I am wondering if prog-id was really the only information that was needed? Is > it meaningful to other tools? Does that correlate to anything in /proc? In > earlier discussion, it sounded like more information was needed to be sure > what was happening. yep, as David mentions in the changelog the global ID is enough, because you can get all the other bpf program info based on that jirka
On Mon, Dec 02, 2019 at 06:00:14PM -0500, Paul Moore wrote: > On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote: > > From: Daniel Borkmann <daniel@iogearbox.net> > > > > Allow for audit messages to be emitted upon BPF program load and > > unload for having a timeline of events. The load itself is in > > syscall context, so additional info about the process initiating > > the BPF prog creation can be logged and later directly correlated > > to the unload event. > > > > The only info really needed from BPF side is the globally unique > > prog ID where then audit user space tooling can query / dump all > > info needed about the specific BPF program right upon load event > > and enrich the record, thus these changes needed here can be kept > > small and non-intrusive to the core. > > > > Raw example output: > > > > # auditctl -D > > # auditctl -a always,exit -F arch=x86_64 -S bpf > > # ausearch --start recent -m 1334 > > ... > > ---- > > time->Wed Nov 27 16:04:13 2019 > > type=PROCTITLE msg=audit(1574867053.120:84664): proctitle="./bpf" > > type=SYSCALL msg=audit(1574867053.120:84664): arch=c000003e syscall=321 \ > > success=yes exit=3 a0=5 a1=7ffea484fbe0 a2=70 a3=0 items=0 ppid=7477 \ > > pid=12698 auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001 \ > > egid=1001 sgid=1001 fsgid=1001 tty=pts2 ses=4 comm="bpf" \ > > exe="/home/jolsa/auditd/audit-testsuite/tests/bpf/bpf" \ > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null) > > type=UNKNOWN[1334] msg=audit(1574867053.120:84664): prog-id=76 op=LOAD > > ---- > > time->Wed Nov 27 16:04:13 2019 > > type=UNKNOWN[1334] msg=audit(1574867053.120:84665): prog-id=76 op=UNLOAD > > ... > > > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > > Co-developed-by: Jiri Olsa <jolsa@kernel.org> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > include/uapi/linux/audit.h | 1 + > > kernel/bpf/syscall.c | 27 +++++++++++++++++++++++++++ > > 2 files changed, 28 insertions(+) > > Hi all, sorry for the delay; the merge window in combination with the > holiday in the US bumped this back a bit. Small comments inline below np, thanks for review > ... > > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -23,6 +23,7 @@ > > #include <linux/timekeeping.h> > > #include <linux/ctype.h> > > #include <linux/nospec.h> > > +#include <linux/audit.h> > > #include <uapi/linux/btf.h> > > > > #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ > > @@ -1306,6 +1307,30 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog) > > return 0; > > } > > > > +enum bpf_audit { > > + BPF_AUDIT_LOAD, > > + BPF_AUDIT_UNLOAD, > > +}; > > + > > +static const char * const bpf_audit_str[] = { > > + [BPF_AUDIT_LOAD] = "LOAD", > > + [BPF_AUDIT_UNLOAD] = "UNLOAD", > > +}; > > + > > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > > +{ > > + struct audit_buffer *ab; > > + > > + if (audit_enabled == AUDIT_OFF) > > + return; > > I think you would probably also want to check the results of > audit_dummy_context() here as well, see all the various audit_XXX() > functions in include/linux/audit.h as an example. You'll see a > pattern similar to the following: > > static inline void audit_foo(...) > { > if (unlikely(!audit_dummy_context())) > __audit_foo(...) > } bpf_audit_prog might be called outside of syscall context for UNLOAD event, so that would prevent it from being stored I can see audit_log_start checks on value of audit_context() that we pass in, should we check for audit_dummy_context just for load event? like: static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) { struct audit_buffer *ab; if (audit_enabled == AUDIT_OFF) return; if (op == BPF_AUDIT_LOAD && audit_dummy_context()) return; ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); if (unlikely(!ab)) return; ... } > > > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); > > + if (unlikely(!ab)) > > + return; > > + audit_log_format(ab, "prog-id=%u op=%s", > > + prog->aux->id, bpf_audit_str[op]); > > Is it worth putting some checks in here to make sure that you don't > blow past the end of the bpf_audit_str array? > > > + audit_log_end(ab); > > +} > > The audit record format looks much better now, thank you. Although I > do wonder if you want bpf_audit_prog() to live in kernel/bpf/syscall.c > or in kernel/auditsc.c? There is plenty of precedence for moving it > into auditsc.c and defining a no-op version for when > CONFIG_AUDITSYSCALL is not enabled, but I personally don't feel that > strongly about either option. I just wanted to mention this in case > you weren't already aware. > > If you do keep it in syscall.c, I don't think there is a need to > implement a no-op version dependent on CONFIG_AUDITSYSCALL; that will > just clutter the code. > > If you do move it to auditsc.c please change the name to > audit_bpf()/__audit_bpf() so it matches the other functions; if you > keep it in syscall.c you can name it whatever you like :) ok, so far I think we'll keep it kernel/bpf/syscall.c thanks, jirka
On Tue, Dec 3, 2019 at 4:38 AM Jiri Olsa <jolsa@redhat.com> wrote: > On Mon, Dec 02, 2019 at 06:00:14PM -0500, Paul Moore wrote: > > On Thu, Nov 28, 2019 at 4:16 AM Jiri Olsa <jolsa@kernel.org> wrote: ... > > > --- a/kernel/bpf/syscall.c > > > +++ b/kernel/bpf/syscall.c > > > @@ -23,6 +23,7 @@ > > > #include <linux/timekeeping.h> > > > #include <linux/ctype.h> > > > #include <linux/nospec.h> > > > +#include <linux/audit.h> > > > #include <uapi/linux/btf.h> > > > > > > #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ > > > @@ -1306,6 +1307,30 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog) > > > return 0; > > > } > > > > > > +enum bpf_audit { > > > + BPF_AUDIT_LOAD, > > > + BPF_AUDIT_UNLOAD, > > > +}; > > > + > > > +static const char * const bpf_audit_str[] = { > > > + [BPF_AUDIT_LOAD] = "LOAD", > > > + [BPF_AUDIT_UNLOAD] = "UNLOAD", > > > +}; > > > + > > > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > > > +{ > > > + struct audit_buffer *ab; > > > + > > > + if (audit_enabled == AUDIT_OFF) > > > + return; > > > > I think you would probably also want to check the results of > > audit_dummy_context() here as well, see all the various audit_XXX() > > functions in include/linux/audit.h as an example. You'll see a > > pattern similar to the following: > > > > static inline void audit_foo(...) > > { > > if (unlikely(!audit_dummy_context())) > > __audit_foo(...) > > } > > bpf_audit_prog might be called outside of syscall context for UNLOAD event, > so that would prevent it from being stored Okay, right. More on this below ... > I can see audit_log_start checks on value of audit_context() that we pass in, The check in audit_log_start() is for a different reason; it checks the passed context to see if it should associate the record with others in the same event, e.g. PATH records associated with the matching SYSCALL record. This way all the associated records show up as part of the same event (as defined by the audit timestamp). > should we check for audit_dummy_context just for load event? like: > > > static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > { > struct audit_buffer *ab; > > if (audit_enabled == AUDIT_OFF) > return; > if (op == BPF_AUDIT_LOAD && audit_dummy_context()) > return; > ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); > if (unlikely(!ab)) > return; > ... > } Ignoring the dummy context for a minute, there is likely a larger issue here with using audit_context() when bpf_audit_prog() is called outside of a syscall, e.g. BPF_AUDIT_UNLOAD. In this case we likely shouldn't be taking the audit context from the current task, we shouldn't be taking it from anywhere, it should be NULL. As far as the dummy context is concerned, you might want to skip the dummy context check since you can only do that on the LOAD side, which means that depending on the system's configuration you could end up with a number of unbalanced LOAD/UNLOAD events. The downside is that you are always going to get the BPF audit records on systemd based systems, since they ignore the admin's audit configuration and always enable audit (yes, we've tried to get systemd to change, they don't seem to care).
On Mon, Dec 02, 2019 at 06:00:14PM -0500, Paul Moore wrote: SNIP > > + > > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > > +{ > > + struct audit_buffer *ab; > > + > > + if (audit_enabled == AUDIT_OFF) > > + return; > > I think you would probably also want to check the results of > audit_dummy_context() here as well, see all the various audit_XXX() > functions in include/linux/audit.h as an example. You'll see a > pattern similar to the following: > > static inline void audit_foo(...) > { > if (unlikely(!audit_dummy_context())) > __audit_foo(...) > } > > > + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); > > + if (unlikely(!ab)) > > + return; > > + audit_log_format(ab, "prog-id=%u op=%s", > > + prog->aux->id, bpf_audit_str[op]); > > Is it worth putting some checks in here to make sure that you don't > blow past the end of the bpf_audit_str array? forgot answer this one.. there are only 2 callers: bpf_audit_prog(prog, BPF_AUDIT_UNLOAD); bpf_audit_prog(prog, BPF_AUDIT_LOAD); that's not going to change any time soon, so I dont think we don't need such check jirka
On Tue, Dec 03, 2019 at 09:53:16PM -0500, Paul Moore wrote: SNIP > > > > > > static inline void audit_foo(...) > > > { > > > if (unlikely(!audit_dummy_context())) > > > __audit_foo(...) > > > } > > > > bpf_audit_prog might be called outside of syscall context for UNLOAD event, > > so that would prevent it from being stored > > Okay, right. More on this below ... > > > I can see audit_log_start checks on value of audit_context() that we pass in, > > The check in audit_log_start() is for a different reason; it checks > the passed context to see if it should associate the record with > others in the same event, e.g. PATH records associated with the > matching SYSCALL record. This way all the associated records show up > as part of the same event (as defined by the audit timestamp). > > > should we check for audit_dummy_context just for load event? like: > > > > > > static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > > { > > struct audit_buffer *ab; > > > > if (audit_enabled == AUDIT_OFF) > > return; > > if (op == BPF_AUDIT_LOAD && audit_dummy_context()) > > return; > > ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); > > if (unlikely(!ab)) > > return; > > ... > > } > > Ignoring the dummy context for a minute, there is likely a larger > issue here with using audit_context() when bpf_audit_prog() is called > outside of a syscall, e.g. BPF_AUDIT_UNLOAD. In this case we likely > shouldn't be taking the audit context from the current task, we > shouldn't be taking it from anywhere, it should be NULL. > > As far as the dummy context is concerned, you might want to skip the > dummy context check since you can only do that on the LOAD side, which > means that depending on the system's configuration you could end up > with a number of unbalanced LOAD/UNLOAD events. The downside is that > you are always going to get the BPF audit records on systemd based > systems, since they ignore the admin's audit configuration and always > enable audit (yes, we've tried to get systemd to change, they don't > seem to care). ok, so something like below? thanks, jirka --- include/uapi/linux/audit.h | 1 + kernel/bpf/syscall.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index c89c6495983d..32a5db900f47 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -116,6 +116,7 @@ #define AUDIT_FANOTIFY 1331 /* Fanotify access decision */ #define AUDIT_TIME_INJOFFSET 1332 /* Timekeeping offset injected */ #define AUDIT_TIME_ADJNTPVAL 1333 /* NTP value adjustment */ +#define AUDIT_BPF 1334 /* BPF subsystem */ #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index e3461ec59570..81f1a6308aa8 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -23,6 +23,7 @@ #include <linux/timekeeping.h> #include <linux/ctype.h> #include <linux/nospec.h> +#include <linux/audit.h> #include <uapi/linux/btf.h> #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ @@ -1306,6 +1307,33 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog) return 0; } +enum bpf_audit { + BPF_AUDIT_LOAD, + BPF_AUDIT_UNLOAD, +}; + +static const char * const bpf_audit_str[] = { + [BPF_AUDIT_LOAD] = "LOAD", + [BPF_AUDIT_UNLOAD] = "UNLOAD", +}; + +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) +{ + struct audit_context *ctx = NULL; + struct audit_buffer *ab; + + if (audit_enabled == AUDIT_OFF) + return; + if (op == BPF_AUDIT_LOAD) + ctx = audit_context(); + ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF); + if (unlikely(!ab)) + return; + audit_log_format(ab, "prog-id=%u op=%s", + prog->aux->id, bpf_audit_str[op]); + audit_log_end(ab); +} + int __bpf_prog_charge(struct user_struct *user, u32 pages) { unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; @@ -1421,6 +1449,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) { if (atomic64_dec_and_test(&prog->aux->refcnt)) { perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0); + bpf_audit_prog(prog, BPF_AUDIT_UNLOAD); /* bpf_prog_free_id() must be called first */ bpf_prog_free_id(prog, do_idr_lock); __bpf_prog_put_noref(prog, true); @@ -1830,6 +1859,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr) */ bpf_prog_kallsyms_add(prog); perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0); + bpf_audit_prog(prog, BPF_AUDIT_LOAD); err = bpf_prog_new_fd(prog); if (err < 0)
On Wed, Dec 4, 2019 at 9:08 AM Jiri Olsa <jolsa@redhat.com> wrote: > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index c89c6495983d..32a5db900f47 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -116,6 +116,7 @@ > #define AUDIT_FANOTIFY 1331 /* Fanotify access decision */ > #define AUDIT_TIME_INJOFFSET 1332 /* Timekeeping offset injected */ > #define AUDIT_TIME_ADJNTPVAL 1333 /* NTP value adjustment */ > +#define AUDIT_BPF 1334 /* BPF subsystem */ > > #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ > #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index e3461ec59570..81f1a6308aa8 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -23,6 +23,7 @@ > #include <linux/timekeeping.h> > #include <linux/ctype.h> > #include <linux/nospec.h> > +#include <linux/audit.h> > #include <uapi/linux/btf.h> > > #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ > @@ -1306,6 +1307,33 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog) > return 0; > } > > +enum bpf_audit { > + BPF_AUDIT_LOAD, > + BPF_AUDIT_UNLOAD, > +}; > + > +static const char * const bpf_audit_str[] = { > + [BPF_AUDIT_LOAD] = "LOAD", > + [BPF_AUDIT_UNLOAD] = "UNLOAD", > +}; > + > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > +{ > + struct audit_context *ctx = NULL; > + struct audit_buffer *ab; > + > + if (audit_enabled == AUDIT_OFF) > + return; > + if (op == BPF_AUDIT_LOAD) > + ctx = audit_context(); > + ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF); > + if (unlikely(!ab)) > + return; > + audit_log_format(ab, "prog-id=%u op=%s", > + prog->aux->id, bpf_audit_str[op]); > + audit_log_end(ab); > +} As mentioned previously, I still think it might be a good idea to ensure "op" is within the bounds of bpf_audit_str, but the audit bits look reasonable to me. > int __bpf_prog_charge(struct user_struct *user, u32 pages) > { > unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > @@ -1421,6 +1449,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) > { > if (atomic64_dec_and_test(&prog->aux->refcnt)) { > perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0); > + bpf_audit_prog(prog, BPF_AUDIT_UNLOAD); > /* bpf_prog_free_id() must be called first */ > bpf_prog_free_id(prog, do_idr_lock); > __bpf_prog_put_noref(prog, true); > @@ -1830,6 +1859,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr) > */ > bpf_prog_kallsyms_add(prog); > perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0); > + bpf_audit_prog(prog, BPF_AUDIT_LOAD); > > err = bpf_prog_new_fd(prog); > if (err < 0) > -- > 2.23.0
On Wed, Dec 04, 2019 at 09:38:10AM -0500, Paul Moore wrote: SNIP > > + > > +static const char * const bpf_audit_str[] = { > > + [BPF_AUDIT_LOAD] = "LOAD", > > + [BPF_AUDIT_UNLOAD] = "UNLOAD", > > +}; > > + > > +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) > > +{ > > + struct audit_context *ctx = NULL; > > + struct audit_buffer *ab; > > + > > + if (audit_enabled == AUDIT_OFF) > > + return; > > + if (op == BPF_AUDIT_LOAD) > > + ctx = audit_context(); > > + ab = audit_log_start(ctx, GFP_ATOMIC, AUDIT_BPF); > > + if (unlikely(!ab)) > > + return; > > + audit_log_format(ab, "prog-id=%u op=%s", > > + prog->aux->id, bpf_audit_str[op]); > > + audit_log_end(ab); > > +} > > As mentioned previously, I still think it might be a good idea to > ensure "op" is within the bounds of bpf_audit_str, but the audit bits > look reasonable to me. ok, I'll add that, I'll send out full patch thanks for the review, jirka
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index c89c6495983d..32a5db900f47 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -116,6 +116,7 @@ #define AUDIT_FANOTIFY 1331 /* Fanotify access decision */ #define AUDIT_TIME_INJOFFSET 1332 /* Timekeeping offset injected */ #define AUDIT_TIME_ADJNTPVAL 1333 /* NTP value adjustment */ +#define AUDIT_BPF 1334 /* BPF subsystem */ #define AUDIT_AVC 1400 /* SE Linux avc denial or grant */ #define AUDIT_SELINUX_ERR 1401 /* Internal SE Linux Errors */ diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index e3461ec59570..20826aad247c 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -23,6 +23,7 @@ #include <linux/timekeeping.h> #include <linux/ctype.h> #include <linux/nospec.h> +#include <linux/audit.h> #include <uapi/linux/btf.h> #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ @@ -1306,6 +1307,30 @@ static int find_prog_type(enum bpf_prog_type type, struct bpf_prog *prog) return 0; } +enum bpf_audit { + BPF_AUDIT_LOAD, + BPF_AUDIT_UNLOAD, +}; + +static const char * const bpf_audit_str[] = { + [BPF_AUDIT_LOAD] = "LOAD", + [BPF_AUDIT_UNLOAD] = "UNLOAD", +}; + +static void bpf_audit_prog(const struct bpf_prog *prog, enum bpf_audit op) +{ + struct audit_buffer *ab; + + if (audit_enabled == AUDIT_OFF) + return; + ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_BPF); + if (unlikely(!ab)) + return; + audit_log_format(ab, "prog-id=%u op=%s", + prog->aux->id, bpf_audit_str[op]); + audit_log_end(ab); +} + int __bpf_prog_charge(struct user_struct *user, u32 pages) { unsigned long memlock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; @@ -1421,6 +1446,7 @@ static void __bpf_prog_put(struct bpf_prog *prog, bool do_idr_lock) { if (atomic64_dec_and_test(&prog->aux->refcnt)) { perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_UNLOAD, 0); + bpf_audit_prog(prog, BPF_AUDIT_UNLOAD); /* bpf_prog_free_id() must be called first */ bpf_prog_free_id(prog, do_idr_lock); __bpf_prog_put_noref(prog, true); @@ -1830,6 +1856,7 @@ static int bpf_prog_load(union bpf_attr *attr, union bpf_attr __user *uattr) */ bpf_prog_kallsyms_add(prog); perf_event_bpf_event(prog, PERF_BPF_EVENT_PROG_LOAD, 0); + bpf_audit_prog(prog, BPF_AUDIT_LOAD); err = bpf_prog_new_fd(prog); if (err < 0)