| 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: <9207911d-65b4-f874-3dcb-52d0c0a4950f@linux.dev>
Date: Mon, 7 Aug 2023 11:09:35 -0700
From: Yonghong Song <yonghong.song@...ux.dev>
To: Eduard Zingerman <eddyz87@...il.com>,
syzbot <syzbot+d61b595e9205573133b3@...kaller.appspotmail.com>,
andrii@...nel.org, ast@...nel.org, bpf@...r.kernel.org,
daniel@...earbox.net, davem@...emloft.net, haoluo@...gle.com,
hawk@...nel.org, john.fastabend@...il.com, jolsa@...nel.org,
kpsingh@...nel.org, kuba@...nel.org, linux-kernel@...r.kernel.org,
martin.lau@...ux.dev, netdev@...r.kernel.org, sdf@...gle.com,
song@...nel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [bpf?] KMSAN: uninit-value in
ieee802154_subif_start_xmit
On 8/7/23 7:45 AM, Eduard Zingerman wrote:
> On Mon, 2023-08-07 at 07:40 -0700, Yonghong Song wrote:
>>
>> On 8/7/23 6:11 AM, Eduard Zingerman wrote:
>>> On Sun, 2023-08-06 at 23:40 -0700, Yonghong Song wrote:
>>>>
>>>> On 8/6/23 4:23 PM, syzbot wrote:
>>>>> Hello,
>>>>>
>>>>> syzbot found the following issue on:
>>>>>
>>>>> HEAD commit: 25ad10658dc1 riscv, bpf: Adapt bpf trampoline to optimized..
>>>>> git tree: bpf-next
>>>>> console+strace: https://syzkaller.appspot.com/x/log.txt?x=147cbb29a80000
>>>>> kernel config: https://syzkaller.appspot.com/x/.config?x=8acaeb93ad7c6aaa
>>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=d61b595e9205573133b3
>>>>> compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
>>>>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14d73ccea80000
>>>>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1276aedea80000
>>>>>
>>>>> Downloadable assets:
>>>>> disk image: https://storage.googleapis.com/syzbot-assets/3d378cc13d42/disk-25ad1065.raw.xz
>>>>> vmlinux: https://storage.googleapis.com/syzbot-assets/44580fd5d1af/vmlinux-25ad1065.xz
>>>>> kernel image: https://storage.googleapis.com/syzbot-assets/840587618b41/bzImage-25ad1065.xz
>>>>>
>>>>> The issue was bisected to:
>>>>>
>>>>> commit 8100928c881482a73ed8bd499d602bab0fe55608
>>>>> Author: Yonghong Song <yonghong.song@...ux.dev>
>>>>> Date: Fri Jul 28 01:12:02 2023 +0000
>>>>>
>>>>> bpf: Support new sign-extension mov insns
>>>>
>>>> Thanks for reporting. I will look into this ASAP.
>>>
>>> Hi Yonghong,
>>>
>>> I guess it's your night and my morning, so I did some initial assessment.
>>> The BPF program being loaded is:
>>>
>>> 0 : (62) *(u32 *)(r10 -8) = 553656332
>>> 1 : (bf) r1 = (s16)r10
>>> 2 : (07) r1 += -8
>>> 3 : (b7) r2 = 3
>>> 4 : (bd) if r2 <= r1 goto pc+0
>>> 5 : (85) call bpf_trace_printk#6
>>> 6 : (b7) r0 = 0
>>> 7 : (95) exit
>>>
>>> (Note: when using bpftool (prog dump xlated id <some-id>) the disassembly
>>> of the instruction #1 is incorrectly printed as "1: (bf) r1 = r10")
>>>
>>> The error occurs when instruction #5 (call to printk) is executed.
>>> An incorrect address for the format string is passed to printk.
>>> Disassembly of the jited program looks as follows:
>>>
>>> $ bpftool prog dump jited id <some-id>
>>> bpf_prog_ebeed182d92b487f:
>>> 0: nopl (%rax,%rax)
>>> 5: nop
>>> 7: pushq %rbp
>>> 8: movq %rsp, %rbp
>>> b: subq $8, %rsp
>>> 12: movl $553656332, -8(%rbp)
>>> 19: movswq %bp, %rdi ; <---- Note movswq %bp !
>>> 1d: addq $-8, %rdi
>>> 21: movl $3, %esi
>>> 26: cmpq %rdi, %rsi
>>> 29: jbe 0x2b
>>> 2b: callq 0xffffffffe11c484c
>>> 30: xorl %eax, %eax
>>> 32: leave
>>> 33: retq
>>>
>>> Note jit instruction #19 corresponding to BPF instruction #1, which
>>> loads truncated and sign-extended value of %rbp's first byte as an
>>> address of format string.
>>>
>>> Here is how verifier log looks for (slightly modified) program:
>>>
>>> func#0 @0
>>> 0: R1=ctx(off=0,imm=0) R10=fp0
>>> ; asm volatile (" \n\
>>> 0: (b7) r1 = 553656332 ; R1_w=553656332
>>> 1: (63) *(u32 *)(r10 -8) = r1 ; R1_w=553656332 R10=fp0 fp-8=553656332
>>> 2: (bf) r1 = (s16)r10 ; R1_w=fp0 R10=fp0
>>> 3: (07) r1 += -8 ; R1_w=fp-8
>>> 4: (b7) r2 = 3 ; R2_w=3
>>> 5: (bd) if r2 <= r1 goto pc+0 ; R1_w=fp-8 R2_w=3
>>> 6: (85) call bpf_trace_printk#6
>>> mark_precise: frame0: last_idx 6 first_idx 0 subseq_idx -1
>>> ...
>>> mark_precise: frame0: falling back to forcing all scalars precise
>>> 7: R0=scalar()
>>> 7: (b7) r0 = 0 ; R0_w=0
>>> 8: (95) exit
>>>
>>> from 5 to 6: R1_w=fp-8 R2_w=3 R10=fp0 fp-8=553656332
>>> 6: (85) call bpf_trace_printk#6
>>> mark_precise: frame0: last_idx 6 first_idx 0 subseq_idx -1
>>> ...
>>> mark_precise: frame0: falling back to forcing all scalars precise
>>> 7: safe
>>>
>>> Note the following line:
>>>
>>> 2: (bf) r1 = (s16)r10 ; R1_w=fp0 R10=fp0
>>>
>>> Verifier incorrectly marked r1 as fp0, hence not noticing the problem
>>> with address passed to printk.
>>
>> Thanks, Eduard. Right. I am also able to dump xlated code like
>> below:
>>
>> 0: (62) *(u32 *)(r10 -8) = 553656332
>> 1: (bf) r1 = (s16)r10
>> 2: (07) r1 += -8
>> 3: (b7) r2 = 3
>> 4: (bd) if r2 <= r1 goto pc+0
>> 5: (85) call bpf_trace_printk#-138320
>> 6: (b7) r0 = 0
>> 7: (95) exit
>>
>> Something like below can fix the problem,
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 132f25dab931..db72619551b2 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -13171,6 +13171,7 @@ static int check_alu_op(struct bpf_verifier_env
>> *env, struct bpf_insn *insn)
>> if (no_sext && need_id)
>> src_reg->id =
>> ++env->id_gen;
>> copy_register_state(dst_reg,
>> src_reg);
>> + dst_reg->type = SCALAR_VALUE;
>> if (!no_sext)
>> dst_reg->id = 0;
>> coerce_reg_to_size_sx(dst_reg,
>> insn->off >> 3);
>>
>> After insn 1, we need change r1 type to SCALAR_VALUE. Will add
>> the the test to selftest and submit the patch to fix the problem
>> today.
>
> Should this be an error?
> Like in the same function but slightly below, when u32 moves are
> processed:
>
> /* R1 = (u32) R2 */
> if (is_pointer_value(env, insn->src_reg)) {
> verbose(env,
> "R%d partial copy of pointer\n",
> insn->src_reg);
> return -EACCES;
> } else { ...
Right, this is indeed better for unprivileged prog run.
I have submitted a patch to fix the issue reported by syzbot.
Please help review. Thanks!
>
>>
>>>
>>> Thanks,
>>> Eduard.
>>>
>>>>>
>>>>> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=17970c5da80000
>>>>> final oops: https://syzkaller.appspot.com/x/report.txt?x=14570c5da80000
>>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=10570c5da80000
>>>>>
>> [...]
>
>
Powered by blists - more mailing lists