[PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Jerome Marchand]

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
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@redhat.com>
---
 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,
 };
-- 
2.49.0

Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Yonghong Song]

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)

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.

> 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@redhat.com>
> ---
>   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,
>   };

Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Eduard Zingerman]

"Jerome Marchand" <jmarchan@redhat.com> writes:

> 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
> 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@redhat.com>
> ---

Looks like we don't run bpf_sysctl_get_name tests on the CI.
CI executes the following binaries:
- test_progs{,-no_alu32,-cpuv4}
- test_verifier
- test_maps
test_progs is what is actively developed.

I agree with the reasoning behind this patch, however, could you please
add a selftest demonstrating unsafe behaviour?
You can use tools/testing/selftests/bpf/progs/verifier_and.c as an
example of verifier test checking for specific log message.
(framework also supports execution if __retval is specified,
 tests can be written in plain C as well, e.g. as in .../iters.c).

Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Jerome Marchand]

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@redhat.com>
>> ---
>>   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,
>>   };
> 
> 

Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Jerome Marchand]

On 27/05/2025 23:39, Eduard Zingerman wrote:
> "Jerome Marchand" <jmarchan@redhat.com> writes:
> 
>> 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
>> 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@redhat.com>
>> ---
> 
> Looks like we don't run bpf_sysctl_get_name tests on the CI.
> CI executes the following binaries:
> - test_progs{,-no_alu32,-cpuv4}
> - test_verifier
> - test_maps
> test_progs is what is actively developed.
> 
> I agree with the reasoning behind this patch, however, could you please
> add a selftest demonstrating unsafe behaviour?

Do you mean to write a selftest that demonstrate the current unsafe 
behavior of the bpf_sysctl_get_name helper? I could write something 
similar as the failing test_sysctl cases.

I'm thinking that a more general test that would check that helpers 
don't access memory in a different way than advertised in their 
prototype would be more useful. But that's quite a different endeavor.


Regards,
Jerome

> You can use tools/testing/selftests/bpf/progs/verifier_and.c as an
> example of verifier test checking for specific log message.
> (framework also supports execution if __retval is specified,
>   tests can be written in plain C as well, e.g. as in .../iters.c).
> 

Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Eduard Zingerman]

Jerome Marchand <jmarchan@redhat.com> writes:

[...]

>> Looks like we don't run bpf_sysctl_get_name tests on the CI.
>> CI executes the following binaries:
>> - test_progs{,-no_alu32,-cpuv4}
>> - test_verifier
>> - test_maps
>> test_progs is what is actively developed.
>> I agree with the reasoning behind this patch, however, could you
>> please
>> add a selftest demonstrating unsafe behaviour?
>
> Do you mean to write a selftest that demonstrate the current unsafe
> behavior of the bpf_sysctl_get_name helper? I could write something
> similar as the failing test_sysctl cases.

Yes, something like that, taking an unsafe action based on content of
the buffer after the helper call.

> I'm thinking that a more general test that would check that helpers
> don't access memory in a different way than advertised in their
> prototype would be more useful. But that's quite a different endeavor.

That would be interesting, I think.
Depends on how much time you need to write such a test.

[...]

Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Yonghong Song]

On 5/28/25 2:09 AM, Jerome Marchand wrote:
> 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.

Thanks. I think ARG_PTR_TO_MEM | MEM_WRITE is better to express the
intention of the helper.

> 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@redhat.com>
>>> ---
>>>   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,
>>>   };
>>
>>
>

Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args

[Jerome Marchand]

On 28/05/2025 18:41, Eduard Zingerman wrote:
> Jerome Marchand <jmarchan@redhat.com> writes:
> 
> [...]
> 
>>> Looks like we don't run bpf_sysctl_get_name tests on the CI.
>>> CI executes the following binaries:
>>> - test_progs{,-no_alu32,-cpuv4}
>>> - test_verifier
>>> - test_maps
>>> test_progs is what is actively developed.
>>> I agree with the reasoning behind this patch, however, could you
>>> please
>>> add a selftest demonstrating unsafe behaviour?
>>
>> Do you mean to write a selftest that demonstrate the current unsafe
>> behavior of the bpf_sysctl_get_name helper? I could write something
>> similar as the failing test_sysctl cases.
> 
> Yes, something like that, taking an unsafe action based on content of
> the buffer after the helper call.

Alright, I'll do that.

> 
>> I'm thinking that a more general test that would check that helpers
>> don't access memory in a different way than advertised in their
>> prototype would be more useful. But that's quite a different endeavor.
> 
> That would be interesting, I think.
> Depends on how much time you need to write such a test.

I might think about it, but ATM, I just don't have time to do that. If 
someone is interested in implementing it, be my guest.

> 
> [...]
> 

[PATCH v2 0/2] bpf: Specify access type of bpf_sysctl_get_name args

[Jerome Marchand]

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") that mistake was hidden by the way the verifier
treated helper accesses. Since then, the verifier, working on wrong
infromation from the prototype, can make faulty optimization that
would had been caught by the test_sysctl selftests if it was run by
the CI.

The first patch fixes bpf_sysctl_get_name prototype.

The second patch converts the test_sysctl to prog_tests so that it
will be run by the CI and catch similar issues in the future.

Changes in v2:
 - Replace ARG_PTR_TO_UNINIT_MEM by ARG_PTR_TO_MEM | MEM_WRITE.
 - Converts test_sysctl to prog_tests.

Jerome Marchand (2):
  bpf: Specify access type of bpf_sysctl_get_name args
  selftests/bpf: Convert test_sysctl to prog_tests

 kernel/bpf/cgroup.c                           |  2 +-
 tools/testing/selftests/bpf/.gitignore        |  1 -
 tools/testing/selftests/bpf/Makefile          |  5 ++-
 .../bpf/{ => prog_tests}/test_sysctl.c        | 32 ++++---------------
 4 files changed, 10 insertions(+), 30 deletions(-)
 rename tools/testing/selftests/bpf/{ => prog_tests}/test_sysctl.c (98%)

-- 
2.49.0

[PATCH v2 1/2] bpf: Specify access type of bpf_sysctl_get_name args

[Jerome Marchand]

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
test_sysctl selftest to the tests related to sysctl_get_name.

Add MEM_WRITE flag the second argument of bpf_sysctl_get_name().

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 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..76994c204b503 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_MEM | MEM_WRITE,
 	.arg3_type	= ARG_CONST_SIZE,
 	.arg4_type	= ARG_ANYTHING,
 };
-- 
2.49.0

[PATCH v2 2/2] selftests/bpf: Convert test_sysctl to prog_tests

[Jerome Marchand]

Convert test_sysctl test to prog_tests with minimal change to the
tests themselves.

Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
---
 tools/testing/selftests/bpf/.gitignore        |  1 -
 tools/testing/selftests/bpf/Makefile          |  5 ++-
 .../bpf/{ => prog_tests}/test_sysctl.c        | 32 ++++---------------
 3 files changed, 9 insertions(+), 29 deletions(-)
 rename tools/testing/selftests/bpf/{ => prog_tests}/test_sysctl.c (98%)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e2a2c46c008b1..3d8378972d26c 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -21,7 +21,6 @@ test_lirc_mode2_user
 flow_dissector_load
 test_tcpnotify_user
 test_libbpf
-test_sysctl
 xdping
 test_cpp
 *.d
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 66bb50356be08..53dc08d905bd1 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -70,7 +70,7 @@ endif
 # Order correspond to 'make run_tests' order
 TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_progs \
 	test_sockmap \
-	test_tcpnotify_user test_sysctl \
+	test_tcpnotify_user \
 	test_progs-no_alu32
 TEST_INST_SUBDIRS := no_alu32
 
@@ -215,7 +215,7 @@ ifeq ($(VMLINUX_BTF),)
 $(error Cannot find a vmlinux for VMLINUX_BTF at any of "$(VMLINUX_BTF_PATHS)")
 endif
 
-# Define simple and short `make test_progs`, `make test_sysctl`, etc targets
+# Define simple and short `make test_progs`, `make test_maps`, etc targets
 # to build individual tests.
 # NOTE: Semicolon at the end is critical to override lib.mk's default static
 # rule for binaries.
@@ -324,7 +324,6 @@ NETWORK_HELPERS := $(OUTPUT)/network_helpers.o
 $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_tcpnotify_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(TRACE_HELPERS)
 $(OUTPUT)/test_sock_fields: $(CGROUP_HELPERS) $(TESTING_HELPERS)
-$(OUTPUT)/test_sysctl: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_tag: $(TESTING_HELPERS)
 $(OUTPUT)/test_lirc_mode2_user: $(TESTING_HELPERS)
 $(OUTPUT)/xdping: $(TESTING_HELPERS)
diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/prog_tests/test_sysctl.c
similarity index 98%
rename from tools/testing/selftests/bpf/test_sysctl.c
rename to tools/testing/selftests/bpf/prog_tests/test_sysctl.c
index bcdbd27f22f08..049671361f8fa 100644
--- a/tools/testing/selftests/bpf/test_sysctl.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_sysctl.c
@@ -1,22 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2019 Facebook
 
-#include <fcntl.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-
-#include <linux/filter.h>
-
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include <bpf/bpf_endian.h>
-#include "bpf_util.h"
+#include "test_progs.h"
 #include "cgroup_helpers.h"
-#include "testing_helpers.h"
 
 #define CG_PATH			"/foo"
 #define MAX_INSNS		512
@@ -1608,26 +1594,22 @@ static int run_tests(int cgfd)
 	return fails ? -1 : 0;
 }
 
-int main(int argc, char **argv)
+void test_sysctl(void)
 {
 	int cgfd = -1;
-	int err = 0;
 
 	cgfd = cgroup_setup_and_join(CG_PATH);
-	if (cgfd < 0)
-		goto err;
+	if (CHECK_FAIL(cgfd < 0))
+		goto out;
 
 	/* Use libbpf 1.0 API mode */
 	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
 
-	if (run_tests(cgfd))
-		goto err;
+	if (CHECK_FAIL(run_tests(cgfd)))
+		goto out;
 
-	goto out;
-err:
-	err = -1;
 out:
 	close(cgfd);
 	cleanup_cgroup_environment();
-	return err;
+	return;
 }
-- 
2.49.0

Re: [PATCH v2 1/2] bpf: Specify access type of bpf_sysctl_get_name args

[Yonghong Song]

On 6/10/25 2:19 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
> test_sysctl selftest to the tests related to sysctl_get_name.
>
> Add MEM_WRITE flag the second argument of bpf_sysctl_get_name().
>
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>

Acked-by: Yonghong Song <yonghong.song@linux.dev>

Re: [PATCH v2 2/2] selftests/bpf: Convert test_sysctl to prog_tests

[Yonghong Song]

On 6/10/25 2:19 AM, Jerome Marchand wrote:
> Convert test_sysctl test to prog_tests with minimal change to the
> tests themselves.
>
> Signed-off-by: Jerome Marchand <jmarchan@redhat.com>
> ---
>   tools/testing/selftests/bpf/.gitignore        |  1 -
>   tools/testing/selftests/bpf/Makefile          |  5 ++-
>   .../bpf/{ => prog_tests}/test_sysctl.c        | 32 ++++---------------
>   3 files changed, 9 insertions(+), 29 deletions(-)
>   rename tools/testing/selftests/bpf/{ => prog_tests}/test_sysctl.c (98%)
>
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index e2a2c46c008b1..3d8378972d26c 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -21,7 +21,6 @@ test_lirc_mode2_user
>   flow_dissector_load
>   test_tcpnotify_user
>   test_libbpf
> -test_sysctl
>   xdping
>   test_cpp
>   *.d
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 66bb50356be08..53dc08d905bd1 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -70,7 +70,7 @@ endif
>   # Order correspond to 'make run_tests' order
>   TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_progs \
>   	test_sockmap \
> -	test_tcpnotify_user test_sysctl \
> +	test_tcpnotify_user \
>   	test_progs-no_alu32
>   TEST_INST_SUBDIRS := no_alu32
>   
> @@ -215,7 +215,7 @@ ifeq ($(VMLINUX_BTF),)
>   $(error Cannot find a vmlinux for VMLINUX_BTF at any of "$(VMLINUX_BTF_PATHS)")
>   endif
>   
> -# Define simple and short `make test_progs`, `make test_sysctl`, etc targets
> +# Define simple and short `make test_progs`, `make test_maps`, etc targets
>   # to build individual tests.
>   # NOTE: Semicolon at the end is critical to override lib.mk's default static
>   # rule for binaries.
> @@ -324,7 +324,6 @@ NETWORK_HELPERS := $(OUTPUT)/network_helpers.o
>   $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>   $(OUTPUT)/test_tcpnotify_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(TRACE_HELPERS)
>   $(OUTPUT)/test_sock_fields: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> -$(OUTPUT)/test_sysctl: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>   $(OUTPUT)/test_tag: $(TESTING_HELPERS)
>   $(OUTPUT)/test_lirc_mode2_user: $(TESTING_HELPERS)
>   $(OUTPUT)/xdping: $(TESTING_HELPERS)
> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/prog_tests/test_sysctl.c
> similarity index 98%
> rename from tools/testing/selftests/bpf/test_sysctl.c
> rename to tools/testing/selftests/bpf/prog_tests/test_sysctl.c
> index bcdbd27f22f08..049671361f8fa 100644
> --- a/tools/testing/selftests/bpf/test_sysctl.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_sysctl.c
> @@ -1,22 +1,8 @@
>   // SPDX-License-Identifier: GPL-2.0
>   // Copyright (c) 2019 Facebook
>   
> -#include <fcntl.h>
> -#include <stdint.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <unistd.h>
> -
> -#include <linux/filter.h>
> -
> -#include <bpf/bpf.h>
> -#include <bpf/libbpf.h>
> -
> -#include <bpf/bpf_endian.h>
> -#include "bpf_util.h"
> +#include "test_progs.h"
>   #include "cgroup_helpers.h"
> -#include "testing_helpers.h"
>   
>   #define CG_PATH			"/foo"
>   #define MAX_INSNS		512
> @@ -1608,26 +1594,22 @@ static int run_tests(int cgfd)
>   	return fails ? -1 : 0;
>   }
>   
> -int main(int argc, char **argv)
> +void test_sysctl(void)
>   {
>   	int cgfd = -1;

-1 is not needed.

> -	int err = 0;
>   
>   	cgfd = cgroup_setup_and_join(CG_PATH);
> -	if (cgfd < 0)
> -		goto err;
> +	if (CHECK_FAIL(cgfd < 0))

Use ASSERT* macros. For example, if (!ASSERT_OK_FD(cgfd, "create cgroup"))

> +		goto out;
>   
>   	/* Use libbpf 1.0 API mode */
>   	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);

This is not needed.

>   
> -	if (run_tests(cgfd))
> -		goto err;
> +	if (CHECK_FAIL(run_tests(cgfd)))

if (!ASSERT_OK(run_tests(cgfd), "run_tests"))

> +		goto out;
>   
> -	goto out;
> -err:
> -	err = -1;
>   out:
>   	close(cgfd);
>   	cleanup_cgroup_environment();
> -	return err;
> +	return;
>   }