Re: [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 19/4/24 12:21, Peter Maydell wrote:
> On Fri, 19 Apr 2024 at 10:37, 姜智伟 <jiangzw@tecorigin.com> wrote:
>> Peter Maydell wrote:
>>> I feel like this might be a bug elsewhere. Can you provide
>>> a repro binary and command line?
>>
>> The test file has been attached with RISCV CBO instruction as the first instruction to execute, with command-line arguments as
>> ./build/qemu-system-riscv64 -M virt -smp 1 -nographic -bios crash_test.bin
> 
> It looks like you're building without --enable-debug. If you do
> that then you'll find that we hit an assert in the debug version
> of the function, which your patch doesn't touch:
> 
> #6  0x00007ffff4b90e96 in __GI___assert_fail
>      (assertion=0x55555639a508 "o < sizeof(TCGTemp) *
> tcg_ctx->nb_temps", file=0x5555563995d5 "../../tcg/tcg.c", line=1940,
> function=0x55555639c000 <__PRETTY_FUNCTION__.60> "tcgv_i32_temp") at
> ./assert/assert.c:101
> #7  0x000055555613104c in tcgv_i32_temp (v=0x0) at ../../tcg/tcg.c:1940
> #8  0x0000555555d0882b in tcgv_i64_temp (v=0x0) at
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/include/tcg/tcg.h:638
> #9  0x0000555555d0c17b in gen_helper_cbo_inval (arg1=0x2a8, arg2=0x0)
> at ../../target/riscv/helper.h:121
> #10 0x0000555555d7be65 in trans_cbo_inval (ctx=0x7fffef1c8e50,
> a=0x7fffef1c8cf0) at
> ../../target/riscv/insn_trans/trans_rvzicbo.c.inc:48
> #11 0x0000555555d41f4f in decode_insn32 (ctx=0x7fffef1c8e50,
> insn=8207) at libqemu-riscv64-softmmu.fa.p/decode-insn32.c.inc:2332
> #12 0x0000555555d925f1 in decode_opc (env=0x555556d53e30,
> ctx=0x7fffef1c8e50, opcode=8207) at
> ../../target/riscv/translate.c:1165
> #13 0x0000555555d92ab4 in riscv_tr_translate_insn
> (dcbase=0x7fffef1c8e50, cpu=0x555556d51670) at
> ../../target/riscv/translate.c:1236
> 
> This happens because we've been passed in 0 as our TCGv,
> which isn't valid. That in turn is because trans_cbo_inval()
> does:
>             gen_helper_cbo_inval(tcg_env, cpu_gpr[a->rs1]);
> but a->rs1 is 0.
> 
> The comment in riscv_translate_init() says:
>      /*
>       * cpu_gpr[0] is a placeholder for the zero register. Do not use it.
>       * Use the gen_set_gpr and get_gpr helper functions when accessing regs,
>       * unless you specifically block reads/writes to reg 0.
>       */
> 
> trans_cbo_inval() doesn't do either of those things, so that is
> where your bug is.

Our minds crossed =)

We need to use get_address() to get an address from cpu_gpr[],
since $zero is "special" (NULL).

I'm about to post this fix:

-- >8 --
diff --git a/target/riscv/insn_trans/trans_rvzicbo.c.inc 
b/target/riscv/insn_trans/trans_rvzicbo.c.inc
index d5d7095903..6f6b29598d 100644
--- a/target/riscv/insn_trans/trans_rvzicbo.c.inc
+++ b/target/riscv/insn_trans/trans_rvzicbo.c.inc
@@ -31,27 +31,27 @@
  static bool trans_cbo_clean(DisasContext *ctx, arg_cbo_clean *a)
  {
      REQUIRE_ZICBOM(ctx);
-    gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
+    gen_helper_cbo_clean_flush(tcg_env, get_address(ctx, a->rs1, 0));
      return true;
  }

  static bool trans_cbo_flush(DisasContext *ctx, arg_cbo_flush *a)
  {
      REQUIRE_ZICBOM(ctx);
-    gen_helper_cbo_clean_flush(tcg_env, cpu_gpr[a->rs1]);
+    gen_helper_cbo_clean_flush(tcg_env, get_address(ctx, a->rs1, 0));
      return true;
  }

  static bool trans_cbo_inval(DisasContext *ctx, arg_cbo_inval *a)
  {
      REQUIRE_ZICBOM(ctx);
-    gen_helper_cbo_inval(tcg_env, cpu_gpr[a->rs1]);
+    gen_helper_cbo_inval(tcg_env, get_address(ctx, a->rs1, 0));
      return true;
  }

  static bool trans_cbo_zero(DisasContext *ctx, arg_cbo_zero *a)
  {
      REQUIRE_ZICBOZ(ctx);
-    gen_helper_cbo_zero(tcg_env, cpu_gpr[a->rs1]);
+    gen_helper_cbo_zero(tcg_env, get_address(ctx, a->rs1, 0));
      return true;
  }
---