All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: linux-audit@redhat.com, bpf@vger.kernel.org
Cc: Alexei Starovoitov <ast@kernel.org>,
	Burn Alting <burn.alting@iinet.net.au>
Subject: [PATCH] bpf: restore the ebpf audit UNLOAD id field
Date: Wed, 21 Dec 2022 19:13:43 -0500	[thread overview]
Message-ID: <20221222001343.489117-1-paul@paul-moore.com> (raw)

When changing the ebpf program put() routines to support being called
from within IRQ context the program ID was reset to zero prior to
generating the audit UNLOAD record, which obviously rendered the ID
field bogus (always zero).  This patch resolves this by adding a new
field, bpf_prog_aux::id_audit, which is set when the ebpf program is
allocated an ID and never reset, ensuring a valid ID field,
regardless of the state of the original ID field, bpf_prox_aud::id.

I also modified the bpf_audit_prog() logic used to associate the
AUDIT_BPF record with other associated records, e.g. @ctx != NULL.
Instead of keying off the operation, it now keys off the execution
context, e.g. '!in_irg && !irqs_disabled()', which is much more
appropriate and should help better connect the UNLOAD operations with
the associated audit state (other audit records).

As an note to future bug hunters, I did briefly consider removing the
ID reset in bpf_prog_free_id(), as it would seem that once the
program is removed from the idr pool it can no longer be found by its
ID value, but commit ad8ad79f4f60 ("bpf: offload: free program id
when device disappears") seems to imply that it is beneficial to
reset the ID value.  Perhaps as a secondary indicator that the ebpf
program is unbound/orphaned.

Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
Reported-by: Burn Alting <burn.alting@iinet.net.au>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 include/linux/bpf.h  | 1 +
 kernel/bpf/syscall.c | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9e7d46d16032..a22001ceb2c3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1103,6 +1103,7 @@ struct bpf_prog_aux {
 	u32 max_tp_access;
 	u32 stack_depth;
 	u32 id;
+	u32 id_audit; /* preserves the id for use by audit */
 	u32 func_cnt; /* used by non-func prog as the number of func progs */
 	u32 func_idx; /* 0 for non-func prog, the index in func array for func prog */
 	u32 attach_btf_id; /* in-kernel BTF type id to attach to */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7b373a5e861f..3ec09f4dba18 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1958,13 +1958,13 @@ static void bpf_audit_prog(const struct bpf_prog *prog, unsigned int op)
 		return;
 	if (audit_enabled == AUDIT_OFF)
 		return;
-	if (op == BPF_AUDIT_LOAD)
+	if (!in_irq() && !irqs_disabled())
 		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]);
+			 prog->aux->id_audit, bpf_audit_str[op]);
 	audit_log_end(ab);
 }
 
@@ -1975,8 +1975,10 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
 	idr_preload(GFP_KERNEL);
 	spin_lock_bh(&prog_idr_lock);
 	id = idr_alloc_cyclic(&prog_idr, prog, 1, INT_MAX, GFP_ATOMIC);
-	if (id > 0)
+	if (id > 0) {
 		prog->aux->id = id;
+		prog->aux->id_audit = id;
+	}
 	spin_unlock_bh(&prog_idr_lock);
 	idr_preload_end();
 
-- 
2.39.0


WARNING: multiple messages have this Message-ID (diff)
From: Paul Moore <paul@paul-moore.com>
To: linux-audit@redhat.com, bpf@vger.kernel.org
Cc: Burn Alting <burn.alting@iinet.net.au>,
	Alexei Starovoitov <ast@kernel.org>
Subject: [PATCH] bpf: restore the ebpf audit UNLOAD id field
Date: Wed, 21 Dec 2022 19:13:43 -0500	[thread overview]
Message-ID: <20221222001343.489117-1-paul@paul-moore.com> (raw)

When changing the ebpf program put() routines to support being called
from within IRQ context the program ID was reset to zero prior to
generating the audit UNLOAD record, which obviously rendered the ID
field bogus (always zero).  This patch resolves this by adding a new
field, bpf_prog_aux::id_audit, which is set when the ebpf program is
allocated an ID and never reset, ensuring a valid ID field,
regardless of the state of the original ID field, bpf_prox_aud::id.

I also modified the bpf_audit_prog() logic used to associate the
AUDIT_BPF record with other associated records, e.g. @ctx != NULL.
Instead of keying off the operation, it now keys off the execution
context, e.g. '!in_irg && !irqs_disabled()', which is much more
appropriate and should help better connect the UNLOAD operations with
the associated audit state (other audit records).

As an note to future bug hunters, I did briefly consider removing the
ID reset in bpf_prog_free_id(), as it would seem that once the
program is removed from the idr pool it can no longer be found by its
ID value, but commit ad8ad79f4f60 ("bpf: offload: free program id
when device disappears") seems to imply that it is beneficial to
reset the ID value.  Perhaps as a secondary indicator that the ebpf
program is unbound/orphaned.

Fixes: d809e134be7a ("bpf: Prepare bpf_prog_put() to be called from irq context.")
Reported-by: Burn Alting <burn.alting@iinet.net.au>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 include/linux/bpf.h  | 1 +
 kernel/bpf/syscall.c | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 9e7d46d16032..a22001ceb2c3 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1103,6 +1103,7 @@ struct bpf_prog_aux {
 	u32 max_tp_access;
 	u32 stack_depth;
 	u32 id;
+	u32 id_audit; /* preserves the id for use by audit */
 	u32 func_cnt; /* used by non-func prog as the number of func progs */
 	u32 func_idx; /* 0 for non-func prog, the index in func array for func prog */
 	u32 attach_btf_id; /* in-kernel BTF type id to attach to */
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7b373a5e861f..3ec09f4dba18 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1958,13 +1958,13 @@ static void bpf_audit_prog(const struct bpf_prog *prog, unsigned int op)
 		return;
 	if (audit_enabled == AUDIT_OFF)
 		return;
-	if (op == BPF_AUDIT_LOAD)
+	if (!in_irq() && !irqs_disabled())
 		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]);
+			 prog->aux->id_audit, bpf_audit_str[op]);
 	audit_log_end(ab);
 }
 
@@ -1975,8 +1975,10 @@ static int bpf_prog_alloc_id(struct bpf_prog *prog)
 	idr_preload(GFP_KERNEL);
 	spin_lock_bh(&prog_idr_lock);
 	id = idr_alloc_cyclic(&prog_idr, prog, 1, INT_MAX, GFP_ATOMIC);
-	if (id > 0)
+	if (id > 0) {
 		prog->aux->id = id;
+		prog->aux->id_audit = id;
+	}
 	spin_unlock_bh(&prog_idr_lock);
 	idr_preload_end();
 
-- 
2.39.0

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit


             reply	other threads:[~2022-12-22  0:13 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-22  0:13 Paul Moore [this message]
2022-12-22  0:13 ` [PATCH] bpf: restore the ebpf audit UNLOAD id field Paul Moore
2022-12-22 17:19 ` sdf
2022-12-22 17:19   ` sdf
2022-12-22 19:03   ` Paul Moore
2022-12-22 19:03     ` Paul Moore
2022-12-22 19:40     ` sdf
2022-12-22 19:40       ` sdf
2022-12-22 19:59       ` Paul Moore
2022-12-22 19:59         ` Paul Moore
2022-12-22 20:07         ` Paul Moore
2022-12-22 20:07           ` Paul Moore
2022-12-22 21:27           ` Stanislav Fomichev
2022-12-22 21:27             ` Stanislav Fomichev
2022-12-23 15:30             ` Paul Moore
2022-12-23 15:30               ` Paul Moore
2022-12-22 23:20     ` Jiri Olsa
2022-12-22 23:20       ` Jiri Olsa
2022-12-23 15:37       ` Paul Moore
2022-12-23 15:37         ` Paul Moore
2022-12-23 15:58         ` Paul Moore
2022-12-23 15:58           ` Paul Moore
2022-12-23 18:03           ` Jiri Olsa
2022-12-23 18:03             ` Jiri Olsa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221222001343.489117-1-paul@paul-moore.com \
    --to=paul@paul-moore.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=burn.alting@iinet.net.au \
    --cc=linux-audit@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.