bpf: Fix an incorrect verification success with movsx insn

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>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Link: https://lore.kernel.org/r/20230807175721.671696-1-yonghong.song@linux.dev
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 132f25dab9318698ff72def1aff6b38375277af6..4ccca1f6c9981d8ade3fa4d07398b732068c0ede 100644 (file)
--- 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 */