On 27/05/2025 21:56, Yonghong Song wrote:
On 5/27/25 9:54 AM, Jerome Marchand wrote:
The second argument of bpf_sysctl_get_name() helper is a pointer to a
buffer that is being written to. However that isn't specify in the
prototype.
Until commit 37cce22dbd51a ("bpf: verifier: Refactor helper access
type tracking"), all helper accesses were considered as a possible
write access by the verifier, so no big harm was done. However, since
then, the verifier might make wrong asssumption about the content of
that address which might lead it to make faulty optimizations (such as
removing code that was wrongly labeled dead). This is what happens in
Could you give more detailed example about the above statement?
the verifier might make wrong asssumption about the content of
that address which might lead it to make faulty optimizations (such as
removing code that was wrongly labeled dead)
To be clear, I don't mean that the verifier does anything wrong in this
case. It makes a wrong assumption because it was fed wrong information
by the helper prototype. Here is the output of the verifier with commit
37cce22dbd51a:
func#0 @0
Live regs before insn:
0: .1........ (bf) r7 = r10
1: .1.....7.. (07) r7 += -8
2: .1.....7.. (b7) r0 = 0
3: 01.....7.. (7b) *(u64 *)(r7 +0) = r0
4: .1.....7.. (bf) r2 = r7
5: .12....7.. (b7) r3 = 8
6: .123...7.. (b7) r4 = 1
7: .1234..7.. (85) call bpf_sysctl_get_name#101
8: 0......7.. (55) if r0 != 0x7 goto pc+6
9: .......7.. (18) r8 = 0x6d656d5f706374
11: .......78. (79) r9 = *(u64 *)(r7 +0)
12: ........89 (5d) if r8 != r9 goto pc+2
13: .......... (b7) r0 = 1
14: 0......... (05) goto pc+1
15: .......... (b7) r0 = 0
16: 0......... (95) exit
0: R1=ctx() R10=fp0
0: (bf) r7 = r10 ; R7_w=fp0 R10=fp0
1: (07) r7 += -8 ; R7_w=fp-8
2: (b7) r0 = 0 ; R0_w=0
3: (7b) *(u64 *)(r7 +0) = r0 ; R0_w=0 R7_w=fp-8 fp-8_w=0
4: (bf) r2 = r7 ; R2_w=fp-8 R7_w=fp-8
5: (b7) r3 = 8 ; R3_w=8
6: (b7) r4 = 1 ; R4_w=1
7: (85) call bpf_sysctl_get_name#101
mark_precise: frame0: last_idx 7 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r3 stack= before 6: (b7) r4 = 1
mark_precise: frame0: regs=r3 stack= before 5: (b7) r3 = 8
8: R0_w=scalar()
8: (55) if r0 != 0x7 goto pc+6 ; R0_w=7
9: (18) r8 = 0x6d656d5f706374 ; R8_w=0x6d656d5f706374
11: (79) r9 = *(u64 *)(r7 +0) ; R7=fp-8 R9=0 fp-8=0
12: (5d) if r8 != r9 goto pc+2
mark_precise: frame0: last_idx 12 first_idx 12 subseq_idx -1
mark_precise: frame0: parent state regs=r8 stack=: R0_w=7 R7_w=fp-8 R8_rw=P0x6d656d5f706374 R9_rw=0 R10=fp0 fp-8_w=0
mark_precise: frame0: last_idx 11 first_idx 0 subseq_idx 12
mark_precise: frame0: regs=r8 stack= before 11: (79) r9 = *(u64 *)(r7 +0)
mark_precise: frame0: regs=r8 stack= before 9: (18) r8 = 0x6d656d5f706374
mark_precise: frame0: last_idx 12 first_idx 12 subseq_idx -1
mark_precise: frame0: parent state regs=r9 stack=: R0_w=7 R7_w=fp-8 R8_rw=P0x6d656d5f706374 R9_rw=P0 R10=fp0 fp-8_w=0
mark_precise: frame0: last_idx 11 first_idx 0 subseq_idx 12
mark_precise: frame0: regs=r9 stack= before 11: (79) r9 = *(u64 *)(r7 +0)
mark_precise: frame0: regs= stack=-8 before 9: (18) r8 = 0x6d656d5f706374
mark_precise: frame0: regs= stack=-8 before 8: (55) if r0 != 0x7 goto pc+6
mark_precise: frame0: regs= stack=-8 before 7: (85) call bpf_sysctl_get_name#101
mark_precise: frame0: regs= stack=-8 before 6: (b7) r4 = 1
mark_precise: frame0: regs= stack=-8 before 5: (b7) r3 = 8
mark_precise: frame0: regs= stack=-8 before 4: (bf) r2 = r7
mark_precise: frame0: regs= stack=-8 before 3: (7b) *(u64 *)(r7 +0) = r0
mark_precise: frame0: regs=r0 stack= before 2: (b7) r0 = 0
12: R8=0x6d656d5f706374 R9=0
15: (b7) r0 = 0 ; R0_w=0
16: (95) exit
mark_precise: frame0: last_idx 16 first_idx 12 subseq_idx -1
mark_precise: frame0: regs=r0 stack= before 15: (b7) r0 = 0
from 8 to 15: R0_w=scalar() R7_w=fp-8 R10=fp0 fp-8_w=0
15: R0_w=scalar() R7_w=fp-8 R10=fp0 fp-8_w=0
15: (b7) r0 = 0 ; R0_w=0
16: (95) exit
mark_precise: frame0: last_idx 16 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r0 stack= before 15: (b7) r0 = 0
processed 16 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
At line 11, it still assume that fp-8=0, despite the call to
bpf_sysctl_get_name. Because of that, it assumes that the first branch
of the conditional jump at line 12 is never taken an the program always
return 0 (access denied).
For comparison, here's the verifier output when commit 37cce22dbd51a is
reverted:
func#0 @0
Live regs before insn:
0: .1........ (bf) r7 = r10
1: .1.....7.. (07) r7 += -8
2: .1.....7.. (b7) r0 = 0
3: 01.....7.. (7b) *(u64 *)(r7 +0) = r0
4: .1.....7.. (bf) r2 = r7
5: .12....7.. (b7) r3 = 8
6: .123...7.. (b7) r4 = 1
7: .1234..7.. (85) call bpf_sysctl_get_name#101
8: 0......7.. (55) if r0 != 0x7 goto pc+6
9: .......7.. (18) r8 = 0x6d656d5f706374
11: .......78. (79) r9 = *(u64 *)(r7 +0)
12: ........89 (5d) if r8 != r9 goto pc+2
13: .......... (b7) r0 = 1
14: 0......... (05) goto pc+1
15: .......... (b7) r0 = 0
16: 0......... (95) exit
0: R1=ctx() R10=fp0
0: (bf) r7 = r10 ; R7_w=fp0 R10=fp0
1: (07) r7 += -8 ; R7_w=fp-8
2: (b7) r0 = 0 ; R0_w=0
3: (7b) *(u64 *)(r7 +0) = r0 ; R0_w=0 R7_w=fp-8 fp-8_w=0
4: (bf) r2 = r7 ; R2_w=fp-8 R7_w=fp-8
5: (b7) r3 = 8 ; R3_w=8
6: (b7) r4 = 1 ; R4_w=1
7: (85) call bpf_sysctl_get_name#101
mark_precise: frame0: last_idx 7 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r3 stack= before 6: (b7) r4 = 1
mark_precise: frame0: regs=r3 stack= before 5: (b7) r3 = 8
8: R0_w=scalar()
8: (55) if r0 != 0x7 goto pc+6 ; R0_w=7
9: (18) r8 = 0x6d656d5f706374 ; R8_w=0x6d656d5f706374
11: (79) r9 = *(u64 *)(r7 +0) ; R7=fp-8 R9=scalar() fp-8=mmmmmmmm
12: (5d) if r8 != r9 goto pc+2 ; R8=0x6d656d5f706374 R9=0x6d656d5f706374
13: (b7) r0 = 1 ; R0_w=1
14: (05) goto pc+1
16: (95) exit
mark_precise: frame0: last_idx 16 first_idx 12 subseq_idx -1
mark_precise: frame0: regs=r0 stack= before 14: (05) goto pc+1
mark_precise: frame0: regs=r0 stack= before 13: (b7) r0 = 1
from 12 to 15: R0=7 R7=fp-8 R8=0x6d656d5f706374 R9=scalar() R10=fp0 fp-8=mmmmmmmm
15: R0=7 R7=fp-8 R8=0x6d656d5f706374 R9=scalar() R10=fp0 fp-8=mmmmmmmm
15: (b7) r0 = 0 ; R0_w=0
16: (95) exit
mark_precise: frame0: last_idx 16 first_idx 12 subseq_idx -1
mark_precise: frame0: regs=r0 stack= before 15: (b7) r0 = 0
from 8 to 15: R0_w=scalar() R7_w=fp-8 R10=fp0 fp-8_w=mmmmmmmm
15: R0_w=scalar() R7_w=fp-8 R10=fp0 fp-8_w=mmmmmmmm
15: (b7) r0 = 0 ; R0_w=0
16: (95) exit
mark_precise: frame0: last_idx 16 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r0 stack= before 15: (b7) r0 = 0
processed 19 insns (limit 1000000) max_states_per_insn 0 total_states 1 peak_states 1 mark_read 1
This patch actually may cause a behavior change.
Without this patch, typically the whole buffer will be initialized
to 0 and then the helper itself will copy bytes until seeing a '\0'.
With this patch, bpf prog does not need to initialize the buffer.
Inside the helper, the copied bytes may not cover the whole buffer.
If that's an issue, it could be easily fixed by replacing
ARG_PTR_TO_UNINIT_MEM by ARG_PTR_TO_MEM | MEM_WRITE.
I don't know what the original intention was when bpf_sysctl_get_name()
was introduced, but almost all helpers use ARG_PTR_TO_UNINIT_MEM for
such a case.
Jerome
test_sysctl selftest to the tests related to sysctl_get_name.
Correctly mark the second argument of bpf_sysctl_get_name() as
ARG_PTR_TO_UNINIT_MEM.
Signed-off-by: Jerome Marchand <jmarchan@xxxxxxxxxx>
---
kernel/bpf/cgroup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 84f58f3d028a3..09c02a592d24a 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -2104,7 +2104,7 @@ static const struct bpf_func_proto
bpf_sysctl_get_name_proto = {
.gpl_only = false,
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_CTX,
- .arg2_type = ARG_PTR_TO_MEM,
+ .arg2_type = ARG_PTR_TO_UNINIT_MEM,
.arg3_type = ARG_CONST_SIZE,
.arg4_type = ARG_ANYTHING,
};
