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