Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bc69afd6-6eec-a070-ab96-05ab137aaf0b@linux.dev>
Date:   Mon, 7 Aug 2023 07:40:12 -0700
MIME-Version: 1.0
Reply-To: yonghong.song@linux.dev
Subject: Re: [syzbot] [bpf?] KMSAN: uninit-value in
 ieee802154_subif_start_xmit
Content-Language: en-US
To:     Eduard Zingerman <eddyz87@gmail.com>,
        syzbot <syzbot+d61b595e9205573133b3@syzkaller.appspotmail.com>,
        andrii@kernel.org, ast@kernel.org, bpf@vger.kernel.org,
        daniel@iogearbox.net, davem@davemloft.net, haoluo@google.com,
        hawk@kernel.org, john.fastabend@gmail.com, jolsa@kernel.org,
        kpsingh@kernel.org, kuba@kernel.org, linux-kernel@vger.kernel.org,
        martin.lau@linux.dev, netdev@vger.kernel.org, sdf@google.com,
        song@kernel.org, syzkaller-bugs@googlegroups.com
References: <0000000000002098bc0602496cc3@google.com>
 <d520bd6c-bfd3-47f1-c794-ab451905256b@linux.dev>
 <9c8f04a0bf90db4bb8e6192824ab71f58244b74b.camel@gmail.com>
From:   Yonghong Song <yonghong.song@linux.dev>
In-Reply-To: <9c8f04a0bf90db4bb8e6192824ab71f58244b74b.camel@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@linux.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.

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