* [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access
@ 2024-12-12 9:20 Kumar Kartikeya Dwivedi
2024-12-12 9:20 ` [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members Kumar Kartikeya Dwivedi
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-12 9:20 UTC (permalink / raw)
To: bpf
Cc: kkd, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Eduard Zingerman, Robert Morris, kernel-team
This set fixes a issue reported for tracing and struct ops programs
using btf_ctx_access for ctx checks, where loading a pointer argument
from the ctx doesn't enforce a BPF_DW access size check. The original
report is at link [0]. Also add a regression test along with the fix.
[0]: https://lore.kernel.org/bpf/51338.1732985814@localhost
Kumar Kartikeya Dwivedi (2):
bpf: Check size for BTF-based ctx access of pointer members
selftests/bpf: Add test for narrow ctx load for pointer args
kernel/bpf/btf.c | 6 +++
.../bpf/progs/verifier_btf_ctx_access.c | 40 ++++++++++++++++++-
.../selftests/bpf/progs/verifier_d_path.c | 4 +-
3 files changed, 46 insertions(+), 4 deletions(-)
base-commit: 7d0d673627e20cfa3b21a829a896ce03b58a4f1c
--
2.43.5
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members
2024-12-12 9:20 [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access Kumar Kartikeya Dwivedi
@ 2024-12-12 9:20 ` Kumar Kartikeya Dwivedi
2024-12-12 19:49 ` Eduard Zingerman
2024-12-12 9:20 ` [PATCH bpf v1 2/2] selftests/bpf: Add test for narrow ctx load for pointer args Kumar Kartikeya Dwivedi
2024-12-12 19:50 ` [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access patchwork-bot+netdevbpf
2 siblings, 1 reply; 5+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-12 9:20 UTC (permalink / raw)
To: bpf
Cc: kkd, Robert Morris, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Eduard Zingerman, kernel-team
Robert Morris reported the following program type which passes the
verifier in [0]:
SEC("struct_ops/bpf_cubic_init")
void BPF_PROG(bpf_cubic_init, struct sock *sk)
{
asm volatile("r2 = *(u16*)(r1 + 0)"); // verifier should demand u64
asm volatile("*(u32 *)(r2 +1504) = 0"); // 1280 in some configs
}
The second line may or may not work, but the first instruction shouldn't
pass, as it's a narrow load into the context structure of the struct ops
callback. The code falls back to btf_ctx_access to ensure correctness
and obtaining the types of pointers. Ensure that the size of the access
is correctly checked to be 8 bytes, otherwise the verifier thinks the
narrow load obtained a trusted BTF pointer and will permit loads/stores
as it sees fit.
Perform the check on size after we've verified that the load is for a
pointer field, as for scalar values narrow loads are fine. Access to
structs passed as arguments to a BPF program are also treated as
scalars, therefore no adjustment is needed in their case.
Existing verifier selftests are broken by this change, but because they
were incorrect. Verifier tests for d_path were performing narrow load
into context to obtain path pointer, had this program actually run it
would cause a crash. The same holds for verifier_btf_ctx_access tests.
[0]: https://lore.kernel.org/bpf/51338.1732985814@localhost
Fixes: 9e15db66136a ("bpf: Implement accurate raw_tp context access via BTF")
Reported-by: Robert Morris <rtm@mit.edu>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
kernel/bpf/btf.c | 6 ++++++
tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c | 4 ++--
tools/testing/selftests/bpf/progs/verifier_d_path.c | 4 ++--
3 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index e7a59e6462a9..a63a03582f02 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6543,6 +6543,12 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
return false;
}
+ if (size != sizeof(u64)) {
+ bpf_log(log, "func '%s' size %d must be 8\n",
+ tname, size);
+ return false;
+ }
+
/* check for PTR_TO_RDONLY_BUF_OR_NULL or PTR_TO_RDWR_BUF_OR_NULL */
for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
const struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];
diff --git a/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c b/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
index a570e48b917a..bfc3bf18fed4 100644
--- a/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
+++ b/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
@@ -11,7 +11,7 @@ __success __retval(0)
__naked void btf_ctx_access_accept(void)
{
asm volatile (" \
- r2 = *(u32*)(r1 + 8); /* load 2nd argument value (int pointer) */\
+ r2 = *(u64 *)(r1 + 8); /* load 2nd argument value (int pointer) */\
r0 = 0; \
exit; \
" ::: __clobber_all);
@@ -23,7 +23,7 @@ __success __retval(0)
__naked void ctx_access_u32_pointer_accept(void)
{
asm volatile (" \
- r2 = *(u32*)(r1 + 0); /* load 1nd argument value (u32 pointer) */\
+ r2 = *(u64 *)(r1 + 0); /* load 1nd argument value (u32 pointer) */\
r0 = 0; \
exit; \
" ::: __clobber_all);
diff --git a/tools/testing/selftests/bpf/progs/verifier_d_path.c b/tools/testing/selftests/bpf/progs/verifier_d_path.c
index ec79cbcfde91..87e51a215558 100644
--- a/tools/testing/selftests/bpf/progs/verifier_d_path.c
+++ b/tools/testing/selftests/bpf/progs/verifier_d_path.c
@@ -11,7 +11,7 @@ __success __retval(0)
__naked void d_path_accept(void)
{
asm volatile (" \
- r1 = *(u32*)(r1 + 0); \
+ r1 = *(u64 *)(r1 + 0); \
r2 = r10; \
r2 += -8; \
r6 = 0; \
@@ -31,7 +31,7 @@ __failure __msg("helper call is not allowed in probe")
__naked void d_path_reject(void)
{
asm volatile (" \
- r1 = *(u32*)(r1 + 0); \
+ r1 = *(u64 *)(r1 + 0); \
r2 = r10; \
r2 += -8; \
r6 = 0; \
--
2.43.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH bpf v1 2/2] selftests/bpf: Add test for narrow ctx load for pointer args
2024-12-12 9:20 [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access Kumar Kartikeya Dwivedi
2024-12-12 9:20 ` [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members Kumar Kartikeya Dwivedi
@ 2024-12-12 9:20 ` Kumar Kartikeya Dwivedi
2024-12-12 19:50 ` [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-12-12 9:20 UTC (permalink / raw)
To: bpf
Cc: kkd, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Eduard Zingerman, Robert Morris, kernel-team
Ensure that performing narrow ctx loads other than size == 8 are
rejected when the argument is a pointer type.
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
.../bpf/progs/verifier_btf_ctx_access.c | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c b/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
index bfc3bf18fed4..28b939572cda 100644
--- a/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
+++ b/tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c
@@ -29,4 +29,40 @@ __naked void ctx_access_u32_pointer_accept(void)
" ::: __clobber_all);
}
+SEC("fentry/bpf_fentry_test9")
+__description("btf_ctx_access u32 pointer reject u32")
+__failure __msg("size 4 must be 8")
+__naked void ctx_access_u32_pointer_reject_32(void)
+{
+ asm volatile (" \
+ r2 = *(u32 *)(r1 + 0); /* load 1st argument with narrow load */\
+ r0 = 0; \
+ exit; \
+" ::: __clobber_all);
+}
+
+SEC("fentry/bpf_fentry_test9")
+__description("btf_ctx_access u32 pointer reject u16")
+__failure __msg("size 2 must be 8")
+__naked void ctx_access_u32_pointer_reject_16(void)
+{
+ asm volatile (" \
+ r2 = *(u16 *)(r1 + 0); /* load 1st argument with narrow load */\
+ r0 = 0; \
+ exit; \
+" ::: __clobber_all);
+}
+
+SEC("fentry/bpf_fentry_test9")
+__description("btf_ctx_access u32 pointer reject u8")
+__failure __msg("size 1 must be 8")
+__naked void ctx_access_u32_pointer_reject_8(void)
+{
+ asm volatile (" \
+ r2 = *(u8 *)(r1 + 0); /* load 1st argument with narrow load */\
+ r0 = 0; \
+ exit; \
+" ::: __clobber_all);
+}
+
char _license[] SEC("license") = "GPL";
--
2.43.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members
2024-12-12 9:20 ` [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members Kumar Kartikeya Dwivedi
@ 2024-12-12 19:49 ` Eduard Zingerman
0 siblings, 0 replies; 5+ messages in thread
From: Eduard Zingerman @ 2024-12-12 19:49 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi, bpf
Cc: kkd, Robert Morris, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, kernel-team
On Thu, 2024-12-12 at 01:20 -0800, Kumar Kartikeya Dwivedi wrote:
> Robert Morris reported the following program type which passes the
> verifier in [0]:
>
> SEC("struct_ops/bpf_cubic_init")
> void BPF_PROG(bpf_cubic_init, struct sock *sk)
> {
> asm volatile("r2 = *(u16*)(r1 + 0)"); // verifier should demand u64
> asm volatile("*(u32 *)(r2 +1504) = 0"); // 1280 in some configs
> }
>
> The second line may or may not work, but the first instruction shouldn't
> pass, as it's a narrow load into the context structure of the struct ops
> callback. The code falls back to btf_ctx_access to ensure correctness
> and obtaining the types of pointers. Ensure that the size of the access
> is correctly checked to be 8 bytes, otherwise the verifier thinks the
> narrow load obtained a trusted BTF pointer and will permit loads/stores
> as it sees fit.
>
> Perform the check on size after we've verified that the load is for a
> pointer field, as for scalar values narrow loads are fine. Access to
> structs passed as arguments to a BPF program are also treated as
> scalars, therefore no adjustment is needed in their case.
>
> Existing verifier selftests are broken by this change, but because they
> were incorrect. Verifier tests for d_path were performing narrow load
> into context to obtain path pointer, had this program actually run it
> would cause a crash. The same holds for verifier_btf_ctx_access tests.
>
> [0]: https://lore.kernel.org/bpf/51338.1732985814@localhost
>
> Fixes: 9e15db66136a ("bpf: Implement accurate raw_tp context access via BTF")
> Reported-by: Robert Morris <rtm@mit.edu>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> kernel/bpf/btf.c | 6 ++++++
> tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c | 4 ++--
> tools/testing/selftests/bpf/progs/verifier_d_path.c | 4 ++--
> 3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index e7a59e6462a9..a63a03582f02 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6543,6 +6543,12 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> return false;
> }
>
> + if (size != sizeof(u64)) {
> + bpf_log(log, "func '%s' size %d must be 8\n",
Nit: the error message is somewhat confusing.
Maybe print something like:
"func '%s' param #%d access size should be 8, not %d"?
> + tname, size);
> + return false;
> + }
> +
> /* check for PTR_TO_RDONLY_BUF_OR_NULL or PTR_TO_RDWR_BUF_OR_NULL */
> for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
> const struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];
[...]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access
2024-12-12 9:20 [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access Kumar Kartikeya Dwivedi
2024-12-12 9:20 ` [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members Kumar Kartikeya Dwivedi
2024-12-12 9:20 ` [PATCH bpf v1 2/2] selftests/bpf: Add test for narrow ctx load for pointer args Kumar Kartikeya Dwivedi
@ 2024-12-12 19:50 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-12-12 19:50 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: bpf, kkd, ast, andrii, daniel, martin.lau, eddyz87, rtm,
kernel-team
Hello:
This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:
On Thu, 12 Dec 2024 01:20:48 -0800 you wrote:
> This set fixes a issue reported for tracing and struct ops programs
> using btf_ctx_access for ctx checks, where loading a pointer argument
> from the ctx doesn't enforce a BPF_DW access size check. The original
> report is at link [0]. Also add a regression test along with the fix.
>
> [0]: https://lore.kernel.org/bpf/51338.1732985814@localhost
>
> [...]
Here is the summary with links:
- [bpf,v1,1/2] bpf: Check size for BTF-based ctx access of pointer members
https://git.kernel.org/bpf/bpf/c/659b9ba7cb2d
- [bpf,v1,2/2] selftests/bpf: Add test for narrow ctx load for pointer args
https://git.kernel.org/bpf/bpf/c/8025731c28be
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-12 19:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-12 9:20 [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access Kumar Kartikeya Dwivedi
2024-12-12 9:20 ` [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members Kumar Kartikeya Dwivedi
2024-12-12 19:49 ` Eduard Zingerman
2024-12-12 9:20 ` [PATCH bpf v1 2/2] selftests/bpf: Add test for narrow ctx load for pointer args Kumar Kartikeya Dwivedi
2024-12-12 19:50 ` [PATCH bpf v1 0/2] Add missing size check for BTF-based ctx access patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox