Re: [PATCH bpf-next 1/2] bpf: Fix an incorrect verification success with movsx insn

[Eduard Zingerman <eddyz87@gmail.com>]

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@syzkaller.appspotmail.com
> Fixes: 8100928c8814 ("bpf: Support new sign-extension mov insns")
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>

All works on my side.
Nitpick: the test case could be simplified.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

> ---
>  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 */