[PATCH bpf-next v1] bpf: Include pid, uid and comm in audit output

[PATCH bpf-next v1] bpf: Include pid, uid and comm in audit output

[Dave Tucker]

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 pid, uid and comm we get output as
follows:

time->Wed Dec 13 21:59:59 2023
type=BPF msg=audit(1702504799.156:99528): pid=27279 uid=0
	comm="new_name" prog-id=50092 op=UNLOAD

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>
---
 kernel/bpf/syscall.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 06320d9abf33..71f418edc014 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,7 +2123,14 @@ 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",
+	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));
+	audit_log_format(ab, " prog-id=%u op=%s",
 			 prog->aux->id, bpf_audit_str[op]);
 	audit_log_end(ab);
 }
-- 
2.43.0

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

[Yafang Shao]

On Thu, Dec 14, 2023 at 8:07 PM Dave Tucker <dave@dtucker.co.uk> 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 pid, uid and comm we get output as
> follows:
>
> time->Wed Dec 13 21:59:59 2023
> type=BPF msg=audit(1702504799.156:99528): pid=27279 uid=0
>         comm="new_name" prog-id=50092 op=UNLOAD

Is it possible to integrate these common details like pid, uid, and
comm into the audit_log_format() function for automatic inclusion? Or
would it be more appropriate to create a new helper function like
audit_log_format_common() dedicated specifically to incorporating
these common details? What are your thoughts on this?

-- 
Regards
Yafang

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

[Yafang Shao]

On Thu, Dec 14, 2023 at 9:13 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 8:07 PM Dave Tucker <dave@dtucker.co.uk> 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 pid, uid and comm we get output as
> > follows:
> >
> > time->Wed Dec 13 21:59:59 2023
> > type=BPF msg=audit(1702504799.156:99528): pid=27279 uid=0
> >         comm="new_name" prog-id=50092 op=UNLOAD
>
> Is it possible to integrate these common details like pid, uid, and
> comm into the audit_log_format() function for automatic inclusion? Or
> would it be more appropriate to create a new helper function like
> audit_log_format_common() dedicated specifically to incorporating
> these common details? What are your thoughts on this?

BTW, bpf prog can be unloaded in irq context. Therefore we can't do it
for BPF_AUDIT_UNLOAD.

-- 
Regards
Yafang

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

[Dave Tucker]

> On 14 Dec 2023, at 13:21, Yafang Shao <laoar.shao@gmail.com> wrote:
> 
> On Thu, Dec 14, 2023 at 9:13 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>> 
>> On Thu, Dec 14, 2023 at 8:07 PM Dave Tucker <dave@dtucker.co.uk> 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 pid, uid and comm we get output as
>>> follows:
>>> 
>>> time->Wed Dec 13 21:59:59 2023
>>> type=BPF msg=audit(1702504799.156:99528): pid=27279 uid=0
>>>        comm="new_name" prog-id=50092 op=UNLOAD
>> 
>> Is it possible to integrate these common details like pid, uid, and
>> comm into the audit_log_format() function for automatic inclusion? Or
>> would it be more appropriate to create a new helper function like
>> audit_log_format_common() dedicated specifically to incorporating
>> these common details? What are your thoughts on this?

There's audit_log_task_info from audit.h which adds everything. My
concern was that it is very verbose and doesn’t appear to be widely
used. I don’t think it warrants a helper function just yet since
we’re only doing audit logging in this one function.

That said, I’m working on a patch series to add audit logging to
bpf_link attach and detach events. I’ll gladly turn that into a
helper then since it would be used in more than one place.

> BTW, bpf prog can be unloaded in irq context. Therefore we can't do it
> for BPF_AUDIT_UNLOAD.

I’ve been running this locally, and occasionally I see unload events
where the comm is “kworker/0:0” - I assume that those are from within
the irq context.

type=BPF msg=audit(1702504511.397:202): pid=1 uid=0
    comm="systemd" prog-id=75 op=LOAD

type=BPF msg=audit(1702504541.516:213): pid=23152 uid=0
    comm="kworker/0:0" prog-id=75 op=UNLOAD

That looks ok to me, but it wouldn’t be too hard to skip adding this
information in the irq context if you’d rather.

- Dave

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

[Yafang Shao]

On Thu, Dec 14, 2023 at 10:11 PM Dave Tucker <dave@dtucker.co.uk> wrote:
>
>
>
> > On 14 Dec 2023, at 13:21, Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Thu, Dec 14, 2023 at 9:13 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >>
> >> On Thu, Dec 14, 2023 at 8:07 PM Dave Tucker <dave@dtucker.co.uk> 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 pid, uid and comm we get output as
> >>> follows:
> >>>
> >>> time->Wed Dec 13 21:59:59 2023
> >>> type=BPF msg=audit(1702504799.156:99528): pid=27279 uid=0
> >>>        comm="new_name" prog-id=50092 op=UNLOAD
> >>
> >> Is it possible to integrate these common details like pid, uid, and
> >> comm into the audit_log_format() function for automatic inclusion? Or
> >> would it be more appropriate to create a new helper function like
> >> audit_log_format_common() dedicated specifically to incorporating
> >> these common details? What are your thoughts on this?
>
> There's audit_log_task_info from audit.h which adds everything. My
> concern was that it is very verbose and doesn’t appear to be widely
> used. I don’t think it warrants a helper function just yet since
> we’re only doing audit logging in this one function.
>
> That said, I’m working on a patch series to add audit logging to
> bpf_link attach and detach events. I’ll gladly turn that into a
> helper then since it would be used in more than one place.
>
> > BTW, bpf prog can be unloaded in irq context. Therefore we can't do it
> > for BPF_AUDIT_UNLOAD.
>
> I’ve been running this locally, and occasionally I see unload events
> where the comm is “kworker/0:0” - I assume that those are from within
> the irq context.
>
> type=BPF msg=audit(1702504511.397:202): pid=1 uid=0
>     comm="systemd" prog-id=75 op=LOAD
>
> type=BPF msg=audit(1702504541.516:213): pid=23152 uid=0
>     comm="kworker/0:0" prog-id=75 op=UNLOAD
>
> That looks ok to me, but it wouldn’t be too hard to skip adding this
> information in the irq context if you’d rather.

I believe we need to skip them. Including random task information
could potentially lead to confusion.

-- 
Regards
Yafang

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

[Andrii Nakryiko]

On Thu, Dec 14, 2023 at 4:07 AM Dave Tucker <dave@dtucker.co.uk> 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 pid, uid and comm we get output as
> follows:
>
> time->Wed Dec 13 21:59:59 2023
> type=BPF msg=audit(1702504799.156:99528): pid=27279 uid=0
>         comm="new_name" prog-id=50092 op=UNLOAD

would emitting program name be useful as well?

>
> 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>
> ---
>  kernel/bpf/syscall.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 06320d9abf33..71f418edc014 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,7 +2123,14 @@ 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",
> +       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));
> +       audit_log_format(ab, " prog-id=%u op=%s",
>                          prog->aux->id, bpf_audit_str[op]);
>         audit_log_end(ab);
>  }
> --
> 2.43.0
>

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

[Alexei Starovoitov]

On Thu, Dec 14, 2023 at 6:33 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Thu, Dec 14, 2023 at 10:11 PM Dave Tucker <dave@dtucker.co.uk> wrote:
> >
> >
> >
> > > On 14 Dec 2023, at 13:21, Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > On Thu, Dec 14, 2023 at 9:13 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >>
> > >> On Thu, Dec 14, 2023 at 8:07 PM Dave Tucker <dave@dtucker.co.uk> 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 pid, uid and comm we get output as
> > >>> follows:
> > >>>
> > >>> time->Wed Dec 13 21:59:59 2023
> > >>> type=BPF msg=audit(1702504799.156:99528): pid=27279 uid=0
> > >>>        comm="new_name" prog-id=50092 op=UNLOAD
> > >>
> > >> Is it possible to integrate these common details like pid, uid, and
> > >> comm into the audit_log_format() function for automatic inclusion? Or
> > >> would it be more appropriate to create a new helper function like
> > >> audit_log_format_common() dedicated specifically to incorporating
> > >> these common details? What are your thoughts on this?
> >
> > There's audit_log_task_info from audit.h which adds everything. My
> > concern was that it is very verbose and doesn’t appear to be widely
> > used. I don’t think it warrants a helper function just yet since
> > we’re only doing audit logging in this one function.
> >
> > That said, I’m working on a patch series to add audit logging to
> > bpf_link attach and detach events. I’ll gladly turn that into a
> > helper then since it would be used in more than one place.
> >
> > > BTW, bpf prog can be unloaded in irq context. Therefore we can't do it
> > > for BPF_AUDIT_UNLOAD.
> >
> > I’ve been running this locally, and occasionally I see unload events
> > where the comm is “kworker/0:0” - I assume that those are from within
> > the irq context.
> >
> > type=BPF msg=audit(1702504511.397:202): pid=1 uid=0
> >     comm="systemd" prog-id=75 op=LOAD
> >
> > type=BPF msg=audit(1702504541.516:213): pid=23152 uid=0
> >     comm="kworker/0:0" prog-id=75 op=UNLOAD
> >
> > That looks ok to me, but it wouldn’t be too hard to skip adding this
> > information in the irq context if you’d rather.
>
> I believe we need to skip them. Including random task information
> could potentially lead to confusion.

Yep. It's broken.
We cannot access current_cred() from here. Current is a random task.

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

[Dave Tucker]

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
  - 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 {
-- 
2.43.0

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

[Yonghong Song]

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 {

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

[Dave Tucker]

> On 15 Dec 2023, at 15:24, Yonghong Song <yonghong.song@linux.dev> wrote:
> 
> 
> 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?

You’re right, my mistake.

> 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 {

include/linux/preempt.h says that in_interrupt is also deprecated.
The condition could also be expressed as in_task().

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

Hmmm. Would storing that information in bpf_prog_aux work?

- Dave

> 
> 
>>   - 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 {

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

[Yonghong Song]

On 12/15/23 8:38 AM, Dave Tucker wrote:
>
>> On 15 Dec 2023, at 15:24, Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>> 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?
> You’re right, my mistake.
>
>> 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 {
> include/linux/preempt.h says that in_interrupt is also deprecated.
> The condition could also be expressed as in_task().

Okay, I see. in_interrupt() probably means !in_task(). The key issue
is in vfree()
===
void vfree(const void *addr)
{
         struct vm_struct *vm;
         int i;

         if (unlikely(in_interrupt())) {
                 vfree_atomic(addr);
                 return;
         }

         BUG_ON(in_nmi());
         kmemleak_free(addr);
         might_sleep();
...
===
where if in interrupt, vfree is atomic() and otherwise, it is not.
So if for task context, there are cases in the kernel where
__bpf_prog_put() is inside rcu critical section and this might
cause the problem. But since this is true for a couple of cases,
so it has not been a big issue yet...

>
>> 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.
> Hmmm. Would storing that information in bpf_prog_aux work?

I think this is more reliable and actually you might get more
useful information, considering once going to kthread, you will
lose task related information.

>
> - Dave
>
>>
>>>    - 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 {
>

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

[Yonghong Song]

On 12/15/23 8:53 AM, Yonghong Song wrote:
>
> On 12/15/23 8:38 AM, Dave Tucker wrote:
>>
>>> On 15 Dec 2023, at 15:24, Yonghong Song <yonghong.song@linux.dev> 
>>> wrote:
>>>
>>>
>>> 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?
>> You’re right, my mistake.
>>
>>> 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 {
>> include/linux/preempt.h says that in_interrupt is also deprecated.
>> The condition could also be expressed as in_task().
>
> Okay, I see. in_interrupt() probably means !in_task(). The key issue
> is in vfree()
> ===
> void vfree(const void *addr)
> {
>         struct vm_struct *vm;
>         int i;
>
>         if (unlikely(in_interrupt())) {
>                 vfree_atomic(addr);
>                 return;
>         }
>
>         BUG_ON(in_nmi());
>         kmemleak_free(addr);
>         might_sleep();
> ...
> ===
> where if in interrupt, vfree is atomic() and otherwise, it is not.
> So if for task context, there are cases in the kernel where
> __bpf_prog_put() is inside rcu critical section and this might
> cause the problem. But since this is true for a couple of cases,
> so it has not been a big issue yet...
>
>>
>>> 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.
>> Hmmm. Would storing that information in bpf_prog_aux work?
>
> I think this is more reliable and actually you might get more
> useful information, considering once going to kthread, you will
> lose task related information.

But for the *current* implementation, you don't need to do that.
You only need to have proper interupt and kthread check in bpf_audit_prog.

>
>>
>> - Dave
>>
>>>
>>>>    - 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 {
>>
>

[PATCH bpf-next v3] bpf: Include pid, uid and comm in audit output

[Dave Tucker]

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:

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();
+		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);
 }
 
-- 
2.43.0

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

[Alexei Starovoitov]

On Fri, Dec 15, 2023 at 9:47 AM Dave Tucker <dave@dtucker.co.uk> 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:
>
> 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.

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

[Daniel Borkmann]

On 12/15/23 6:46 PM, 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:
> 
> 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
> 

Aside from what Alexei mentioned, looking back at commit bae141f54be83
("bpf: Emit audit messages upon successful prog load and unload"), don't
you already have this information at hand? Quote from commit msg:

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

Thanks,
Daniel

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

[Dave Tucker]

> On 15 Dec 2023, at 22:20, Daniel Borkmann <daniel@iogearbox.net> wrote:
> 
> On 12/15/23 6:46 PM, 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:
>> 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
> 
> Aside from what Alexei mentioned, looking back at commit bae141f54be83
> ("bpf: Emit audit messages upon successful prog load and unload"), don't
> you already have this information at hand? Quote from commit msg:
> 
> 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
>      ...

The information is only there if you’ve added an audit rule like the one above
to capture the bpf syscall. With the default rules of `auditctl -a task,never`
(at least that’s the default on my system) all you see is:

---
time->Mon Dec 18 13:51:47 2023
type=BPF msg=audit(1702907507.727:858492): prog-id=965 op=LOAD
----
time->Mon Dec 18 13:51:49 2023
type=BPF msg=audit(1702907509.031:858493): prog-id=965 op=UNLOAD
——

I’d be okay with adding rules to enrich the context if it was possible to 
constrain auditing to only certain bpf_cmds, but I’ve not been successful so
far. The rule `always, exit -F arch=b64 -S bpf -F a0=5` still shows map ops in
the audit logs. On a busy system that floods the audit log with noise and useful
logs can be lost due to log rotation.


- Dave

> Thanks,
> Daniel

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

[Paul Moore]

On Fri, Dec 15, 2023 at 1:00 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Fri, Dec 15, 2023 at 9:47 AM Dave Tucker <dave@dtucker.co.uk> 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>

Assuming you have audit configured to not disable syscall auditing
(most distros disable it by default[1]), you should see a SYSCALL
audit record associated with the bpf(BPF_PROG_LOAD) call.  Below is a
quick example taken from a Fedora Rawhide test system:

type=PROCTITLE msg=audit(12/19/2023 13:44:22.917:1976) :
  proctitle=(systemd)
type=SYSCALL msg=audit(12/19/2023 13:44:22.917:1976) :
  arch=x86_64 syscall=bpf success=yes exit=11
  a0=BPF_PROG_LOAD a1=0x7ffd6c818190 a2=0x90 a3=0x13
  items=0 ppid=1 pid=29898 auid=root uid=root gid=root
  euid=root suid=root fsuid=root egid=root sgid=root fsgid=root
  tty=(none) ses=4 comm=systemd
  exe=/usr/lib/systemd/systemd
  subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=BPF msg=audit(12/19/2023 13:44:22.917:1976) :
  prog-id=128 op=LOAD

You will notice that the full user credentials and process
information, e.g. PID, are captured in the SYSCALL record.
Admittedly, we are missing the BPF program name, which I think could
be a reasonable addition to the BPF record, but I don't want to see us
duplicate information from the SYSCALL record in the BPF record unless
we have a situation where we are loading BPF programs outside a user
syscall event.

[1] https://github.com/linux-audit/audit-documentation/wiki/HOWTO-Fedora-Enable-Auditing

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

Thanks Alexei.  Yes, please send any audit related patches to the
audit mailing list.  It's listed in MAINTAINERS, but I've also added
it to the CC line above.

-- 
paul-moore.com