Re: [PATCH bpf-next v2 1/1] bpf: Include pid, uid and comm in audit output

[Yonghong Song <yonghong.song@linux.dev>]

On 12/15/23 6:38 AM, Dave Tucker 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@dtucker.co.uk>
> ---
>
> Changes:
>
> 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

Is this true? For condition '(in_hardirq() || irqs_disabled())
', the context will be the task context. But what about nmi and
softirq? The context maynot be the task context, right?

Not sure whether this is relevant or not. There was a discussion
in the past about the above condition. See
   https://lore.kernel.org/bpf/a93079f2-fcd4-e3ef-3b92-92d443b8e8c6@meta.com/

The recommended change was

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 
a75c54b6f8a3..11df562e481b 100644 --- a/kernel/bpf/syscall.c +++ 
b/kernel/bpf/syscall.c @@ -2147,7 +2147,7 @@ static void 
__bpf_prog_put(struct bpf_prog *prog)           struct bpf_prog_aux *aux = prog->aux;

          if (atomic64_dec_and_test(&aux->refcnt)) {
- if (in_irq() || irqs_disabled()) { + if (!in_interrupt()) {                           INIT_WORK(&aux->work, bpf_prog_put_deferred);
                          schedule_work(&aux->work);
                  } else {

If the above change was true, then audit will not be able to
get any meaning task specific information, you might need to
gather such informaiton before hand.


>    - Only populate pid, uid and comm if not in a kthread
>
>   kernel/bpf/syscall.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 06320d9abf33..fbbaf3b8ad48 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,18 +2111,34 @@ 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;
>   	if (audit_enabled == AUDIT_OFF)
>   		return;
> -	if (!in_irq() && !irqs_disabled())
> -		ctx = audit_context();
> +
> +	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_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();
> +		audit_log_format(ab, " pid=%u uid=%u",
> +				 task_pid_nr(current),
> +				 from_kuid(&init_user_ns, cred->uid));
> +		audit_log_format(ab, " comm=");
> +		audit_log_untrustedstring(ab, get_task_comm(comm, current));
> +	} else {
> +		audit_log_format(ab, " pid=? uid=? comm=?");
> +	}
>   	audit_log_end(ab);
>   }
>   
> @@ -2212,7 +2229,7 @@ static void __bpf_prog_put(struct bpf_prog *prog)
>   	struct bpf_prog_aux *aux = prog->aux;
>   
>   	if (atomic64_dec_and_test(&aux->refcnt)) {
> -		if (in_irq() || irqs_disabled()) {
> +		if (in_hardirq() || irqs_disabled()) {
>   			INIT_WORK(&aux->work, bpf_prog_put_deferred);
>   			schedule_work(&aux->work);
>   		} else {