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@xxxxxxxxx>
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.
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
[...]
