* [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS
@ 2024-01-12 15:20 Hao Sun
2024-01-12 15:20 ` [PATCH v3 2/2] selftests/bpf: Add tests for " Hao Sun
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Hao Sun @ 2024-01-12 15:20 UTC (permalink / raw)
To: bpf; +Cc: willemb, ast, andrii, daniel, eddyz87, linux-kernel, Hao Sun
For PTR_TO_FLOW_KEYS, check_flow_keys_access() only uses fixed off
for validation. However, variable offset ptr alu is not prohibited
for this ptr kind. So the variable offset is not checked.
The following prog is accepted:
func#0 @0
0: R1=ctx() R10=fp0
0: (bf) r6 = r1 ; R1=ctx() R6_w=ctx()
1: (79) r7 = *(u64 *)(r6 +144) ; R6_w=ctx() R7_w=flow_keys()
2: (b7) r8 = 1024 ; R8_w=1024
3: (37) r8 /= 1 ; R8_w=scalar()
4: (57) r8 &= 1024 ; R8_w=scalar(smin=smin32=0,
smax=umax=smax32=umax32=1024,var_off=(0x0; 0x400))
5: (0f) r7 += r8
mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r8 stack= before 4: (57) r8 &= 1024
mark_precise: frame0: regs=r8 stack= before 3: (37) r8 /= 1
mark_precise: frame0: regs=r8 stack= before 2: (b7) r8 = 1024
6: R7_w=flow_keys(smin=smin32=0,smax=umax=smax32=umax32=1024,var_off
=(0x0; 0x400)) R8_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=1024,
var_off=(0x0; 0x400))
6: (79) r0 = *(u64 *)(r7 +0) ; R0_w=scalar()
7: (95) exit
This prog loads flow_keys to r7, and adds the variable offset r8
to r7, and finally causes out-of-bounds access:
BUG: unable to handle page fault for address: ffffc90014c80038
...
Call Trace:
<TASK>
bpf_dispatcher_nop_func include/linux/bpf.h:1231 [inline]
__bpf_prog_run include/linux/filter.h:651 [inline]
bpf_prog_run include/linux/filter.h:658 [inline]
bpf_prog_run_pin_on_cpu include/linux/filter.h:675 [inline]
bpf_flow_dissect+0x15f/0x350 net/core/flow_dissector.c:991
bpf_prog_test_run_flow_dissector+0x39d/0x620 net/bpf/test_run.c:1359
bpf_prog_test_run kernel/bpf/syscall.c:4107 [inline]
__sys_bpf+0xf8f/0x4560 kernel/bpf/syscall.c:5475
__do_sys_bpf kernel/bpf/syscall.c:5561 [inline]
__se_sys_bpf kernel/bpf/syscall.c:5559 [inline]
__x64_sys_bpf+0x73/0xb0 kernel/bpf/syscall.c:5559
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0x3f/0x110 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x63/0x6b
Fix this by rejecting ptr alu with variable offset on flow_keys.
Applying the patch makes the program rejected with "R7 pointer
arithmetic on flow_keys prohibited"
Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
Signed-off-by: Hao Sun <sunhao.th@gmail.com>
---
kernel/bpf/verifier.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index adbf330d364b..65f598694d55 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -12826,6 +12826,10 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
}
switch (base_type(ptr_reg->type)) {
+ case PTR_TO_FLOW_KEYS:
+ if (known)
+ break;
+ fallthrough;
case CONST_PTR_TO_MAP:
/* smin_val represents the known value */
if (known && smin_val == 0 && opcode == BPF_ADD)
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] selftests/bpf: Add tests for alu on PTR_TO_FLOW_KEYS
2024-01-12 15:20 [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS Hao Sun
@ 2024-01-12 15:20 ` Hao Sun
2024-01-12 18:14 ` Yonghong Song
2024-01-12 17:56 ` [PATCH v3 1/2] bpf: Reject variable offset " Alexei Starovoitov
2024-01-12 18:38 ` Yonghong Song
2 siblings, 1 reply; 6+ messages in thread
From: Hao Sun @ 2024-01-12 15:20 UTC (permalink / raw)
To: bpf; +Cc: willemb, ast, andrii, daniel, eddyz87, linux-kernel, Hao Sun
Add two cases for PTR_TO_FLOW_KEYS alu. One for rejecting alu with
variable offset, another for fixed offset.
Signed-off-by: Hao Sun <sunhao.th@gmail.com>
---
.../bpf/progs/verifier_value_illegal_alu.c | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c b/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
index 71814a753216..3bcccb4cbc85 100644
--- a/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
+++ b/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
@@ -146,4 +146,40 @@ l0_%=: exit; \
: __clobber_all);
}
+SEC("flow_dissector")
+__description("flow_keys illegal alu op with variable offset")
+__failure __msg("R7 pointer arithmetic on flow_keys prohibited")
+__naked void flow_keys_illegal_variable_offset_alu(void)
+{
+ asm volatile(" \
+ r6 = r1; \
+ r7 = *(u64*)(r6 + %[flow_keys_off]); \
+ r8 = 8; \
+ r8 /= 1; \
+ r8 &= 8; \
+ r7 += r8; \
+ r0 = *(u64*)(r7 + 0); \
+ exit; \
+" :
+ : __imm_const(flow_keys_off, offsetof(struct __sk_buff, flow_keys))
+ : __clobber_all);
+}
+
+SEC("flow_dissector")
+__description("flow_keys valid alu op with fixed offset")
+__success
+__naked void flow_keys_legal_fixed_offset_alu(void)
+{
+ asm volatile(" \
+ r6 = r1; \
+ r7 = *(u64*)(r6 + %[flow_keys_off]); \
+ r8 = 8; \
+ r7 += r8; \
+ r0 = *(u64*)(r7 + 0); \
+ exit; \
+" :
+ : __imm_const(flow_keys_off, offsetof(struct __sk_buff, flow_keys))
+ : __clobber_all);
+}
+
char _license[] SEC("license") = "GPL";
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS
2024-01-12 15:20 [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS Hao Sun
2024-01-12 15:20 ` [PATCH v3 2/2] selftests/bpf: Add tests for " Hao Sun
@ 2024-01-12 17:56 ` Alexei Starovoitov
2024-01-12 18:02 ` Hao Sun
2024-01-12 18:38 ` Yonghong Song
2 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2024-01-12 17:56 UTC (permalink / raw)
To: Hao Sun
Cc: bpf, Willem de Bruijn, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Eddy Z, LKML
On Fri, Jan 12, 2024 at 7:20 AM Hao Sun <sunhao.th@gmail.com> wrote:
>
> For PTR_TO_FLOW_KEYS, check_flow_keys_access() only uses fixed off
> for validation. However, variable offset ptr alu is not prohibited
> for this ptr kind. So the variable offset is not checked.
Why resend v3?
What changed from v2?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS
2024-01-12 17:56 ` [PATCH v3 1/2] bpf: Reject variable offset " Alexei Starovoitov
@ 2024-01-12 18:02 ` Hao Sun
0 siblings, 0 replies; 6+ messages in thread
From: Hao Sun @ 2024-01-12 18:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Willem de Bruijn, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Eddy Z, LKML
On Fri, Jan 12, 2024 at 6:57 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jan 12, 2024 at 7:20 AM Hao Sun <sunhao.th@gmail.com> wrote:
> >
> > For PTR_TO_FLOW_KEYS, check_flow_keys_access() only uses fixed off
> > for validation. However, variable offset ptr alu is not prohibited
> > for this ptr kind. So the variable offset is not checked.
>
> Why resend v3?
> What changed from v2?
Nothing changes in the first patch, the tests in the second patch are
reformatted
with a proper number of tabs after each instruction. Forgot to add changelogs.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] selftests/bpf: Add tests for alu on PTR_TO_FLOW_KEYS
2024-01-12 15:20 ` [PATCH v3 2/2] selftests/bpf: Add tests for " Hao Sun
@ 2024-01-12 18:14 ` Yonghong Song
0 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2024-01-12 18:14 UTC (permalink / raw)
To: Hao Sun, bpf; +Cc: willemb, ast, andrii, daniel, eddyz87, linux-kernel
On 1/12/24 7:20 AM, Hao Sun wrote:
> Add two cases for PTR_TO_FLOW_KEYS alu. One for rejecting alu with
> variable offset, another for fixed offset.
>
> Signed-off-by: Hao Sun <sunhao.th@gmail.com>
> ---
> .../bpf/progs/verifier_value_illegal_alu.c | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c b/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
> index 71814a753216..3bcccb4cbc85 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_value_illegal_alu.c
> @@ -146,4 +146,40 @@ l0_%=: exit; \
> : __clobber_all);
> }
>
> +SEC("flow_dissector")
> +__description("flow_keys illegal alu op with variable offset")
> +__failure __msg("R7 pointer arithmetic on flow_keys prohibited")
> +__naked void flow_keys_illegal_variable_offset_alu(void)
> +{
> + asm volatile(" \
> + r6 = r1; \
> + r7 = *(u64*)(r6 + %[flow_keys_off]); \
> + r8 = 8; \
> + r8 /= 1; \
> + r8 &= 8; \
> + r7 += r8; \
> + r0 = *(u64*)(r7 + 0); \
> + exit; \
> +" :
> + : __imm_const(flow_keys_off, offsetof(struct __sk_buff, flow_keys))
> + : __clobber_all);
> +}
> +
> +SEC("flow_dissector")
> +__description("flow_keys valid alu op with fixed offset")
> +__success
> +__naked void flow_keys_legal_fixed_offset_alu(void)
> +{
> + asm volatile(" \
> + r6 = r1; \
> + r7 = *(u64*)(r6 + %[flow_keys_off]); \
> + r8 = 8; \
> + r7 += r8; \
> + r0 = *(u64*)(r7 + 0); \
> + exit; \
> +" :
> + : __imm_const(flow_keys_off, offsetof(struct __sk_buff, flow_keys))
> + : __clobber_all);
> +}
Please remove the above '__success' case from this file. This file,
verifier_value_illegal_alu.c, only contains failure cases.
For the fixed offset, we already have C code to verify,
e.g., in bpf_flow.c.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS
2024-01-12 15:20 [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS Hao Sun
2024-01-12 15:20 ` [PATCH v3 2/2] selftests/bpf: Add tests for " Hao Sun
2024-01-12 17:56 ` [PATCH v3 1/2] bpf: Reject variable offset " Alexei Starovoitov
@ 2024-01-12 18:38 ` Yonghong Song
2 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2024-01-12 18:38 UTC (permalink / raw)
To: Hao Sun, bpf; +Cc: willemb, ast, andrii, daniel, eddyz87, linux-kernel
On 1/12/24 7:20 AM, Hao Sun wrote:
> For PTR_TO_FLOW_KEYS, check_flow_keys_access() only uses fixed off
> for validation. However, variable offset ptr alu is not prohibited
> for this ptr kind. So the variable offset is not checked.
>
> The following prog is accepted:
> func#0 @0
> 0: R1=ctx() R10=fp0
> 0: (bf) r6 = r1 ; R1=ctx() R6_w=ctx()
> 1: (79) r7 = *(u64 *)(r6 +144) ; R6_w=ctx() R7_w=flow_keys()
> 2: (b7) r8 = 1024 ; R8_w=1024
> 3: (37) r8 /= 1 ; R8_w=scalar()
> 4: (57) r8 &= 1024 ; R8_w=scalar(smin=smin32=0,
> smax=umax=smax32=umax32=1024,var_off=(0x0; 0x400))
> 5: (0f) r7 += r8
> mark_precise: frame0: last_idx 5 first_idx 0 subseq_idx -1
> mark_precise: frame0: regs=r8 stack= before 4: (57) r8 &= 1024
> mark_precise: frame0: regs=r8 stack= before 3: (37) r8 /= 1
> mark_precise: frame0: regs=r8 stack= before 2: (b7) r8 = 1024
> 6: R7_w=flow_keys(smin=smin32=0,smax=umax=smax32=umax32=1024,var_off
> =(0x0; 0x400)) R8_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=1024,
> var_off=(0x0; 0x400))
> 6: (79) r0 = *(u64 *)(r7 +0) ; R0_w=scalar()
> 7: (95) exit
>
> This prog loads flow_keys to r7, and adds the variable offset r8
> to r7, and finally causes out-of-bounds access:
>
> BUG: unable to handle page fault for address: ffffc90014c80038
> ...
> Call Trace:
> <TASK>
> bpf_dispatcher_nop_func include/linux/bpf.h:1231 [inline]
> __bpf_prog_run include/linux/filter.h:651 [inline]
> bpf_prog_run include/linux/filter.h:658 [inline]
> bpf_prog_run_pin_on_cpu include/linux/filter.h:675 [inline]
> bpf_flow_dissect+0x15f/0x350 net/core/flow_dissector.c:991
> bpf_prog_test_run_flow_dissector+0x39d/0x620 net/bpf/test_run.c:1359
> bpf_prog_test_run kernel/bpf/syscall.c:4107 [inline]
> __sys_bpf+0xf8f/0x4560 kernel/bpf/syscall.c:5475
> __do_sys_bpf kernel/bpf/syscall.c:5561 [inline]
> __se_sys_bpf kernel/bpf/syscall.c:5559 [inline]
> __x64_sys_bpf+0x73/0xb0 kernel/bpf/syscall.c:5559
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x3f/0x110 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> Fix this by rejecting ptr alu with variable offset on flow_keys.
> Applying the patch makes the program rejected with "R7 pointer
> arithmetic on flow_keys prohibited"
>
> Fixes: d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> Signed-off-by: Hao Sun <sunhao.th@gmail.com>
Acked-by: Yonghong Song <yonghong.song@linux.dev>
> ---
> kernel/bpf/verifier.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index adbf330d364b..65f598694d55 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -12826,6 +12826,10 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
> }
>
> switch (base_type(ptr_reg->type)) {
> + case PTR_TO_FLOW_KEYS:
> + if (known)
> + break;
> + fallthrough;
> case CONST_PTR_TO_MAP:
> /* smin_val represents the known value */
> if (known && smin_val == 0 && opcode == BPF_ADD)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-12 18:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-12 15:20 [PATCH v3 1/2] bpf: Reject variable offset alu on PTR_TO_FLOW_KEYS Hao Sun
2024-01-12 15:20 ` [PATCH v3 2/2] selftests/bpf: Add tests for " Hao Sun
2024-01-12 18:14 ` Yonghong Song
2024-01-12 17:56 ` [PATCH v3 1/2] bpf: Reject variable offset " Alexei Starovoitov
2024-01-12 18:02 ` Hao Sun
2024-01-12 18:38 ` Yonghong Song
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox