* [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps
@ 2024-04-18 10:27 Zhiwei Jiang
2024-04-18 14:58 ` Richard Henderson
0 siblings, 1 reply; 7+ messages in thread
From: Zhiwei Jiang @ 2024-04-18 10:27 UTC (permalink / raw)
To: qemu-devel; +Cc: richard.henderson, pbonzini, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps
2024-04-18 10:27 [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps Zhiwei Jiang
@ 2024-04-18 14:58 ` Richard Henderson
2024-04-19 3:48 ` 回复:[PATCH] " 姜智伟
0 siblings, 1 reply; 7+ messages in thread
From: Richard Henderson @ 2024-04-18 14:58 UTC (permalink / raw)
To: Zhiwei Jiang, qemu-devel; +Cc: pbonzini
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~
^ permalink raw reply [flat|nested] 7+ messages in thread
* 回复:[PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps
2024-04-18 14:58 ` Richard Henderson
@ 2024-04-19 3:48 ` 姜智伟
2024-04-19 9:19 ` [PATCH] " Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: 姜智伟 @ 2024-04-19 3:48 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: pbonzini
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps
2024-04-19 3:48 ` 回复:[PATCH] " 姜智伟
@ 2024-04-19 9:19 ` Peter Maydell
2024-04-19 9:37 ` 回复:[PATCH] " 姜智伟
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2024-04-19 9:19 UTC (permalink / raw)
To: 姜智伟; +Cc: Richard Henderson, qemu-devel, pbonzini
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* 回复:[PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps
2024-04-19 9:19 ` [PATCH] " Peter Maydell
@ 2024-04-19 9:37 ` 姜智伟
2024-04-19 10:21 ` [PATCH] " Peter Maydell
0 siblings, 1 reply; 7+ messages in thread
From: 姜智伟 @ 2024-04-19 9:37 UTC (permalink / raw)
To: Peter Maydell; +Cc: Richard Henderson, qemu-devel, pbonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps
2024-04-19 9:37 ` 回复:[PATCH] " 姜智伟
@ 2024-04-19 10:21 ` Peter Maydell
2024-04-19 11:02 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 7+ messages in thread
From: Peter Maydell @ 2024-04-19 10:21 UTC (permalink / raw)
To: 姜智伟; +Cc: Richard Henderson, qemu-devel, pbonzini
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps
2024-04-19 10:21 ` [PATCH] " Peter Maydell
@ 2024-04-19 11:02 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-04-19 11:02 UTC (permalink / raw)
To: Peter Maydell, 姜智伟
Cc: Richard Henderson, qemu-devel, pbonzini
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;
}
---
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-04-19 11:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-18 10:27 [PATCH] tcg: Fix the overflow in indexing tcg_ctx->temps Zhiwei Jiang
2024-04-18 14:58 ` Richard Henderson
2024-04-19 3:48 ` 回复:[PATCH] " 姜智伟
2024-04-19 9:19 ` [PATCH] " Peter Maydell
2024-04-19 9:37 ` 回复:[PATCH] " 姜智伟
2024-04-19 10:21 ` [PATCH] " Peter Maydell
2024-04-19 11:02 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.