On Mon, 2023-08-07 at 10:57 -0700, Yonghong Song wrote:
> syzbot reports a verifier bug which triggers a runtime panic.
> The test bpf program 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#-138320
> 6: (b7) r0 = 0
> 7: (95) exit
>
> At insn 1, the current implementation keeps 'r1' as a frame pointer,
> which caused later bpf_trace_printk helper call crash since frame
> pointer address is not valid any more. Note that at insn 4,
> the 'pointer vs. scalar' comparison is allowed for privileged
> prog run.
>
> To fix the problem with above insn 1, the fix in the patch adopts
> similar pattern to existing 'R1 = (u32) R2' handling. For unprivileged
> prog run, verification will fail with 'R<num> sign-extension part of pointer'.
> For privileged prog run, the dst_reg 'r1' will be marked as
> an unknown scalar, so later 'bpf_trace_pointk' helper will complain
> since it expected certain pointers.
>
> Reported-by: syzbot+d61b595e9205573133b3@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 8100928c8814 ("bpf: Support new sign-extension mov insns")
> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
All works on my side.
Nitpick: the test case could be simplified.
Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 132f25dab931..4ccca1f6c998 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -13165,17 +13165,26 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
> dst_reg->subreg_def = DEF_NOT_SUBREG;
> } else {
> /* case: R1 = (s8, s16 s32)R2 */
> - bool no_sext;
> -
> - no_sext = src_reg->umax_value < (1ULL << (insn->off - 1));
> - if (no_sext && need_id)
> - src_reg->id = ++env->id_gen;
> - copy_register_state(dst_reg, src_reg);
> - if (!no_sext)
> - dst_reg->id = 0;
> - coerce_reg_to_size_sx(dst_reg, insn->off >> 3);
> - dst_reg->live |= REG_LIVE_WRITTEN;
> - dst_reg->subreg_def = DEF_NOT_SUBREG;
> + if (is_pointer_value(env, insn->src_reg)) {
> + verbose(env,
> + "R%d sign-extension part of pointer\n",
> + insn->src_reg);
> + return -EACCES;
> + } else if (src_reg->type == SCALAR_VALUE) {
> + bool no_sext;
> +
> + no_sext = src_reg->umax_value < (1ULL << (insn->off - 1));
> + if (no_sext && need_id)
> + src_reg->id = ++env->id_gen;
> + copy_register_state(dst_reg, src_reg);
> + if (!no_sext)
> + dst_reg->id = 0;
> + coerce_reg_to_size_sx(dst_reg, insn->off >> 3);
> + dst_reg->live |= REG_LIVE_WRITTEN;
> + dst_reg->subreg_def = DEF_NOT_SUBREG;
> + } else {
> + mark_reg_unknown(env, regs, insn->dst_reg);
> + }
> }
> } else {
> /* R1 = (u32) R2 */
