On Fri, Dec 15, 2023 at 9:47 AM Dave Tucker <dave@xxxxxxxxxxxxx> wrote:
>
> Current output from auditd is as follows:
>
> time->Wed Dec 13 21:39:24 2023
> type=BPF msg=audit(1702503564.519:11241): prog-id=439 op=LOAD
>
> This only tells you that a BPF program was loaded, but without
> any context. If we include the prog-name, pid, uid and comm we get
> output as follows:
>
> time->Wed Dec 13 21:59:59 2023
> type=BPF msg=audit(1702504799.156:99528): op=UNLOAD prog-id=50092
> prog-name="test" pid=27279 uid=0 comm="new_name"
>
> With pid, uid a system administrator has much better context
> over which processes and user loaded which eBPF programs.
> comm is useful since processes may be short-lived.
>
> Signed-off-by: Dave Tucker <dave@xxxxxxxxxxxxx>
> ---
>
> Changes:
>
> v2->v3:
> - Revert replacing in_irq() with in_hardirq()
> - Revert removal of in_irq() check from bpf_audit_prog since it may
> also be called in the sofirq or nmi contexts
>
> v1->v2:
> - Move 'op' to the front of the audit messages
> - Add 'prog-name' to the audit messages
> - Replace deprecated in_irq() with in_hardirq()
> - Remove in_irq() check from bpf_audit_prog since it's always called
> from the task context
> - Only populate pid, uid and comm if not in a kthread
>
> kernel/bpf/syscall.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 06320d9abf33..86600ca1f106 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -35,6 +35,7 @@
> #include <linux/rcupdate_trace.h>
> #include <linux/memcontrol.h>
> #include <linux/trace_events.h>
> +#include <linux/uidgid.h>
>
> #include <net/netfilter/nf_bpf_link.h>
> #include <net/netkit.h>
> @@ -2110,6 +2111,8 @@ static void bpf_audit_prog(const struct bpf_prog *prog, unsigned int op)
> {
> struct audit_context *ctx = NULL;
> struct audit_buffer *ab;
> + const struct cred *cred;
> + char comm[sizeof(current->comm)];
>
> if (WARN_ON_ONCE(op >= BPF_AUDIT_MAX))
> return;
> @@ -2120,8 +2123,22 @@ static void bpf_audit_prog(const struct bpf_prog *prog, unsigned int op)
> 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_format(ab, "op=%s prog-id=%u",
> + bpf_audit_str[op], prog->aux->id);
> + audit_log_format(ab, " prog-name=");
> + audit_log_untrustedstring(ab, prog->aux->name ?: "(none)");
> +
> + if (current->mm) {
> + cred = current_cred();
Is this how you're trying to detect whether it's running
out of workqueue?
You probably need:
if (!current->mm || (current->flags & PF_KTHREAD))
But even with that it looks wrong, since
BPF_AUDIT_UNLOAD message will be _randomly_ unbalanced
with LOAD message.
Last __bpf_prog_put() can happen in either context.
You also need to cc Paul.
He needs to ack it before we can take it.
As it stands I think this change makes the audit worse.
