| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8c58af8e-1e00-4630-b19b-368a02e60ce1@linux.dev>
Date: Wed, 28 May 2025 10:41:44 -0700
From: Yonghong Song <yonghong.song@...ux.dev>
To: Jerome Marchand <jmarchan@...hat.com>, bpf@...r.kernel.org
Cc: Martin KaFai Lau <martin.lau@...ux.dev>,
Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>, linux-kernel@...r.kernel.org,
Eduard Zingerman <eddyz87@...il.com>
Subject: Re: [PATCH] bpf: Specify access type of bpf_sysctl_get_name args
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@...hat.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,
>>> };
>>
>>
>
Powered by blists - more mailing lists