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

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

[Yonghong Song]

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

[PATCH bpf-next 2/2] selftests/bpf: Add a movsx selftest for sign-extension of R10

[Yonghong Song]

A movsx selftest is added for sign-extension of frame pointer R10.
The verification fails for both privileged and unprivileged
prog runs.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../selftests/bpf/progs/verifier_movsx.c      | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_movsx.c b/tools/testing/selftests/bpf/progs/verifier_movsx.c
index 9568089932d7..be6f69a6b659 100644
--- a/tools/testing/selftests/bpf/progs/verifier_movsx.c
+++ b/tools/testing/selftests/bpf/progs/verifier_movsx.c
@@ -198,6 +198,28 @@ l0_%=:							\
 	: __clobber_all);
 }
 
+SEC("socket")
+__description("MOV64SX, S16, R10 Sign Extension")
+__failure __msg("R1 type=scalar expected=fp, pkt, pkt_meta, map_key, map_value, mem, ringbuf_mem, buf, trusted_ptr_")
+__failure_unpriv __msg_unpriv("R10 sign-extension part of pointer")
+__naked void mov64sx_s16_r10(void)
+{
+	asm volatile ("					\
+	r1 = 553656332;					\
+	*(u32 *)(r10 - 8) = r1; 			\
+	r1 = (s16)r10;					\
+	r1 += -8;					\
+	r2 = 3;						\
+	if r2 <= r1 goto l0_%=;				\
+l0_%=:							\
+	call %[bpf_trace_printk];			\
+	r0 = 0;						\
+	exit;						\
+"	:
+	: __imm(bpf_trace_printk)
+	: __clobber_all);
+}
+
 #else
 
 SEC("socket")
-- 
2.34.1

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

[Eduard Zingerman]

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

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:

On Mon,  7 Aug 2023 10:57:21 -0700 you 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
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] bpf: Fix an incorrect verification success with movsx insn
    https://git.kernel.org/bpf/bpf-next/c/db2baf82b098
  - [bpf-next,2/2] selftests/bpf: Add a movsx selftest for sign-extension of R10
    https://git.kernel.org/bpf/bpf-next/c/a5c0a42bd374

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html