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

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

[Zhiwei Jiang]

Sometimes, when the address of the passed TCGTemp *ts variable is the same as tcg_ctx,
the index calculated in the temp_idx function, i.e., ts - tcg_ctx->temps,
can result in a particularly large value, causing overflow in the subsequent array access.

0  0x00007f79590132ac in test_bit (addr=<optimized out>, nr=<optimized out>)
    at /data/system/jiangzw/release_version/qemu8.2/include/qemu/bitops.h:135
1  init_ts_info (ctx=ctx@entry=0x7f794bffe460, ts=0x7f76fc000e00) at ../tcg/optimize.c:148
2  0x00007f7959014b50 in init_arguments (nb_args=2, op=0x7f76fc0101f8, ctx=0x7f794bffe460) at ../tcg/optimize.c:792
3  fold_call (op=0x7f76fc0101f8, ctx=0x7f794bffe460) at ../tcg/optimize.c:1348
4  tcg_optimize (s=<optimized out>) at ../tcg/optimize.c:2369
5  0x00007f7958ffa136 in tcg_gen_code (s=0x7f76fc000e00, tb=0x7f7904202380, pc_start=140741246462840) at ../tcg/tcg.c:6066

Signed-off-by: Zhiwei Jiang <jiangzw@tecorigin.com>
---
 include/tcg/tcg.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 05a1912f8a..4b38d2702d 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -629,7 +629,7 @@ static inline size_t temp_idx(TCGTemp *ts)
  */
 static inline TCGTemp *tcgv_i32_temp(TCGv_i32 v)
 {
-    return (void *)tcg_ctx + (uintptr_t)v;
+    return (void *)tcg_ctx->temps + (uintptr_t)v;
 }
 #endif
 
@@ -681,7 +681,7 @@ static inline TCGArg tcgv_vec_arg(TCGv_vec v)
 static inline TCGv_i32 temp_tcgv_i32(TCGTemp *t)
 {
     (void)temp_idx(t); /* trigger embedded assert */
-    return (TCGv_i32)((void *)t - (void *)tcg_ctx);
+    return (TCGv_i32)((void *)t - (void *)tcg_ctx->temps);
 }
 
 static inline TCGv_i64 temp_tcgv_i64(TCGTemp *t)
-- 
2.17.1

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

[Richard Henderson]

On 4/18/24 03:27, Zhiwei Jiang wrote:
> Sometimes, when the address of the passed TCGTemp *ts variable is the same as tcg_ctx,

Pardon?  When would TCGTemp *ts == TCGContext *tcg_ctx?


> the index calculated in the temp_idx function, i.e., ts - tcg_ctx->temps,
> can result in a particularly large value, causing overflow in the subsequent array access.

Or, assert:

size_t temp_idx(TCGTemp *ts)
{
     ptrdiff_t n = ts - tcg_ctx->temps;
     assert(n >= 0 && n < tcg_ctx->nb_temps);
     return n;
}

>   static inline TCGTemp *tcgv_i32_temp(TCGv_i32 v)
>   {
> -    return (void *)tcg_ctx + (uintptr_t)v;
> +    return (void *)tcg_ctx->temps + (uintptr_t)v;
>   }

This will generate 0 for the first temp, which will test as NULL.



r~

回复：[PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps

[姜智伟]

> On 4/18/24 03:27, Zhiwei Jiang wrote:
> > Sometimes, when the address of the passed TCGTemp *ts variable is the same as tcg_ctx,
> 
> Pardon?  When would TCGTemp *ts == TCGContext *tcg_ctx?
> 
> 
> > the index calculated in the temp_idx function, i.e., ts - tcg_ctx->temps,
> > can result in a particularly large value, causing overflow in the subsequent array access.
> 
> Or, assert:
> 
> size_t temp_idx(TCGTemp *ts)
> {
>      ptrdiff_t n = ts - tcg_ctx->temps;
>      assert(n >= 0 && n < tcg_ctx->nb_temps);
>      return n;
> }
> 
> >   static inline TCGTemp *tcgv_i32_temp(TCGv_i32 v)
> >   {
> > -    return (void *)tcg_ctx + (uintptr_t)v;
> > +    return (void *)tcg_ctx->temps + (uintptr_t)v;
> >   }
> 
> This will generate 0 for the first temp, which will test as NULL.

Hi Richard:
You can reproduce this issue on the latest upstream QEMU version. Using the RISC-V QEMU version, 
if we compile a test program with the first instruction being '.insn r 0xf, 2, 0, x0, x0, x0',that is a RISC-V CBO instruction, 
qemu will crash with a segmentation fault upon execution.

When the first instruction in the program is a CBO instruction,  temp_idx in init_ts_info func returns a very large value, 
causing the subsequent test_bit function to access out-of-bounds memory.

static void init_ts_info(OptContext *ctx, TCGTemp *ts)
{
    size_t idx = temp_idx(ts);
    TempOptInfo *ti;

    if (test_bit(idx, ctx->temps_used.l)) {
        return;
    }
...
I can fix this segmentation fault by applying the modification above, 
and it seems more logical in terms of code logic to match the allocation and indexing of TCGTemp.

Ths

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

[Peter Maydell]

On Fri, 19 Apr 2024 at 04:49, 姜智伟 <jiangzw@tecorigin.com> wrote:
>
> > On 4/18/24 03:27, Zhiwei Jiang wrote:
> > > Sometimes, when the address of the passed TCGTemp *ts variable is the same as tcg_ctx,
> >
> > Pardon?  When would TCGTemp *ts == TCGContext *tcg_ctx?
> >
> >
> > > the index calculated in the temp_idx function, i.e., ts - tcg_ctx->temps,
> > > can result in a particularly large value, causing overflow in the subsequent array access.
> >
> > Or, assert:
> >
> > size_t temp_idx(TCGTemp *ts)
> > {
> >      ptrdiff_t n = ts - tcg_ctx->temps;
> >      assert(n >= 0 && n < tcg_ctx->nb_temps);
> >      return n;
> > }
> >
> > >   static inline TCGTemp *tcgv_i32_temp(TCGv_i32 v)
> > >   {
> > > -    return (void *)tcg_ctx + (uintptr_t)v;
> > > +    return (void *)tcg_ctx->temps + (uintptr_t)v;
> > >   }
> >
> > This will generate 0 for the first temp, which will test as NULL.
>
> Hi Richard:
> You can reproduce this issue on the latest upstream QEMU version. Using the RISC-V QEMU version,
> if we compile a test program with the first instruction being '.insn r 0xf, 2, 0, x0, x0, x0',that is a RISC-V CBO instruction,
> qemu will crash with a segmentation fault upon execution.
>
> When the first instruction in the program is a CBO instruction,  temp_idx in init_ts_info func returns a very large value,
> causing the subsequent test_bit function to access out-of-bounds memory.

I feel like this might be a bug elsewhere. Can you provide
a repro binary and command line?

thanks
-- PMM

回复：[PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps

[姜智伟]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

> > > On 4/18/24 03:27, Zhiwei Jiang wrote:
> > > > Sometimes, when the address of the passed TCGTemp *ts variable is the same as tcg_ctx,
> > >
> > > Pardon?  When would TCGTemp *ts == TCGContext *tcg_ctx?
> > >
> > >
> > > > the index calculated in the temp_idx function, i.e., ts - tcg_ctx->temps,
> > > > can result in a particularly large value, causing overflow in the subsequent array access.
> > >
> > > Or, assert:
> > >
> > > size_t temp_idx(TCGTemp *ts)
> > > {
> > >      ptrdiff_t n = ts - tcg_ctx->temps;
> > >      assert(n >= 0 && n < tcg_ctx->nb_temps);
> > >      return n;
> > > }
> > >
> > > >   static inline TCGTemp *tcgv_i32_temp(TCGv_i32 v)
> > > >   {
> > > > -    return (void *)tcg_ctx + (uintptr_t)v;
> > > > +    return (void *)tcg_ctx->temps + (uintptr_t)v;
> > > >   }
> > >
> > > This will generate 0 for the first temp, which will test as NULL.
> >
> > Hi Richard:
> > You can reproduce this issue on the latest upstream QEMU version. Using the RISC-V QEMU version,
> > if we compile a test program with the first instruction being '.insn r 0xf, 2, 0, x0, x0, x0',that is a RISC-V CBO instruction,
> > qemu will crash with a segmentation fault upon execution.
> >
> > When the first instruction in the program is a CBO instruction,  temp_idx in init_ts_info func returns a very large value,
> > causing the subsequent test_bit function to access out-of-bounds memory.
> 
> 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

[-- Attachment #2: crash_test.bin --]
[-- Type: application/octet-stream, Size: 8360 bytes --]

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

[Peter Maydell]

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.

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

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;
  }
---