1. Patchew
  2. QEMU
  3. View series

[PATCH] target/riscv: Fix PC sync in trans_sspopchk for CFI exception handling

A-Shehab posted 1 patch 1 month, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260730181852.1622-1-ahshehab24@gmail.com
Maintainers: Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Chao Liu <chao.liu@processmission.com>
target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] target/riscv: Fix PC sync in trans_sspopchk for CFI exception handling
Posted by A-Shehab 1 month, 4 weeks ago 
From: Max Chou <max.chou@sifive.com>

Move gen_update_pc call before conditional logic to ensure consistent
PC state regardless of execution path.

Previously, the host instructions generated to update the cpu_pc were
only executed in the failure path when shadow stack validation failed.
This created inconsistent PC synchronization.

This inconsistency caused issues in CF_PCREL mode where subsequent
instructions calculated wrong relative offsets from stale pc_save
values, and could lead to incorrect exception return addresses.

This fix ensures PC is always synchronized before any helper that
might raise an exception, maintaining consistent translator state
across all execution paths.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4118
Signed-off-by: Max Chou <max.chou@sifive.com>
[ahshehab: rebased on current master; file moved to
 target/riscv/tcg/insn_trans/ and the ssp load is now 64-bit wide]
Tested-by: A-Shehab <ahshehab24@gmail.com>
Signed-off-by: A-Shehab <ahshehab24@gmail.com>
---
This is a repost of Max Chou's patch from 2025-11-05 [1], which did not
receive any review. Rebased onto current master: the file moved to
target/riscv/tcg/insn_trans/ and the ssp load is now a 64-bit load, so
the original patch no longer applies.

I opened a GitLab issue (#4118) with a minimal, self-contained bare-metal
reproducer for this bug. It runs an sspopchk that matches the shadow stack
(the common case) followed by an auipc in the same translation block; on
current master the auipc returns an address 4 bytes too low (exit 42) and
with this patch it is correct (exit 0). The reproducer is included in the
issue.

[1] https://lore.kernel.org/qemu-devel/20251105134331.2865581-1-max.chou@sifive.com/

 target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
index a813232887..d47a9f9c7d 100644
--- a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
+++ b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
@@ -32,6 +32,7 @@ static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a)
     TCGLabel *skip = gen_new_label();
     uint32_t tmp = (get_xl(ctx) == MXL_RV64) ? 8 : 4;
     TCGv data = tcg_temp_new();
+    gen_update_pc(ctx, 0);
     TCGv_i64 wide_addr = tcg_temp_new_i64();
     tcg_gen_ld_i64(wide_addr, tcg_env, offsetof(CPURISCVState, ssp));
     tcg_gen_trunc_i64_tl(addr, wide_addr);
@@ -42,7 +43,6 @@ static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a)
     tcg_gen_brcond_tl(TCG_COND_EQ, data, rs1, skip);
     tcg_gen_st8_i32(tcg_constant_i32(RISCV_EXCP_SW_CHECK_BCFI_TVAL),
                     tcg_env, offsetof(CPURISCVState, sw_check_code));
-    gen_update_pc(ctx, 0);
     gen_helper_raise_exception(tcg_env,
                   tcg_constant_i32(RISCV_EXCP_SW_CHECK));
     gen_set_label(skip);
-- 
2.53.0
Re: [PATCH] target/riscv: Fix PC sync in trans_sspopchk for CFI exception handling
Posted by Michael Tokarev 1 month ago 
On 7/30/26 21:18, A-Shehab wrote:
> From: Max Chou <max.chou@sifive.com>
> 
> Move gen_update_pc call before conditional logic to ensure consistent
> PC state regardless of execution path.
> 
> Previously, the host instructions generated to update the cpu_pc were
> only executed in the failure path when shadow stack validation failed.
> This created inconsistent PC synchronization.
> 
> This inconsistency caused issues in CF_PCREL mode where subsequent
> instructions calculated wrong relative offsets from stale pc_save
> values, and could lead to incorrect exception return addresses.
> 
> This fix ensures PC is always synchronized before any helper that
> might raise an exception, maintaining consistent translator state
> across all execution paths.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4118
> Signed-off-by: Max Chou <max.chou@sifive.com>
> [ahshehab: rebased on current master; file moved to
>   target/riscv/tcg/insn_trans/ and the ssp load is now 64-bit wide]
> Tested-by: A-Shehab <ahshehab24@gmail.com>
> Signed-off-by: A-Shehab <ahshehab24@gmail.com>

I'm applying this to the current qemu stable series.
In 11.0.x and 10.0.x, it is the original version of this patch
(though maybe it is more productive to pick up other changes in
this area to older stable branches and apply this change as-is).

Please let me know if I shouldn't pick it up.

Thanks,

/mjt
Re: [PATCH] target/riscv: Fix PC sync in trans_sspopchk for CFI exception handling
Posted by Alistair 1 month, 2 weeks ago 
On Thu, 2026-07-30 at 21:18 +0300, A-Shehab wrote:
> From: Max Chou <max.chou@sifive.com>
> 
> Move gen_update_pc call before conditional logic to ensure consistent
> PC state regardless of execution path.
> 
> Previously, the host instructions generated to update the cpu_pc were
> only executed in the failure path when shadow stack validation
> failed.
> This created inconsistent PC synchronization.
> 
> This inconsistency caused issues in CF_PCREL mode where subsequent
> instructions calculated wrong relative offsets from stale pc_save
> values, and could lead to incorrect exception return addresses.
> 
> This fix ensures PC is always synchronized before any helper that
> might raise an exception, maintaining consistent translator state
> across all execution paths.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4118
> Signed-off-by: Max Chou <max.chou@sifive.com>
> [ahshehab: rebased on current master; file moved to
>  target/riscv/tcg/insn_trans/ and the ssp load is now 64-bit wide]
> Tested-by: A-Shehab <ahshehab24@gmail.com>
> Signed-off-by: A-Shehab <ahshehab24@gmail.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
> This is a repost of Max Chou's patch from 2025-11-05 [1], which did
> not
> receive any review. Rebased onto current master: the file moved to
> target/riscv/tcg/insn_trans/ and the ssp load is now a 64-bit load,
> so
> the original patch no longer applies.
> 
> I opened a GitLab issue (#4118) with a minimal, self-contained bare-
> metal
> reproducer for this bug. It runs an sspopchk that matches the shadow
> stack
> (the common case) followed by an auipc in the same translation block;
> on
> current master the auipc returns an address 4 bytes too low (exit 42)
> and
> with this patch it is correct (exit 0). The reproducer is included in
> the
> issue.
> 
> [1]
> https://lore.kernel.org/qemu-devel/20251105134331.2865581-1-max.chou@sifive.com/
> 
>  target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> index a813232887..d47a9f9c7d 100644
> --- a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> +++ b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> @@ -32,6 +32,7 @@ static bool trans_sspopchk(DisasContext *ctx,
> arg_sspopchk *a)
>      TCGLabel *skip = gen_new_label();
>      uint32_t tmp = (get_xl(ctx) == MXL_RV64) ? 8 : 4;
>      TCGv data = tcg_temp_new();
> +    gen_update_pc(ctx, 0);
>      TCGv_i64 wide_addr = tcg_temp_new_i64();
>      tcg_gen_ld_i64(wide_addr, tcg_env, offsetof(CPURISCVState,
> ssp));
>      tcg_gen_trunc_i64_tl(addr, wide_addr);
> @@ -42,7 +43,6 @@ static bool trans_sspopchk(DisasContext *ctx,
> arg_sspopchk *a)
>      tcg_gen_brcond_tl(TCG_COND_EQ, data, rs1, skip);
>      tcg_gen_st8_i32(tcg_constant_i32(RISCV_EXCP_SW_CHECK_BCFI_TVAL),
>                      tcg_env, offsetof(CPURISCVState,
> sw_check_code));
> -    gen_update_pc(ctx, 0);
>      gen_helper_raise_exception(tcg_env,
>                    tcg_constant_i32(RISCV_EXCP_SW_CHECK));
>      gen_set_label(skip);
Re: [PATCH] target/riscv: Fix PC sync in trans_sspopchk for CFI exception handling
Posted by Max Chou 1 month, 3 weeks ago 
Thank you A-Shehab
I forgot to ping and update this patch

On Fri, Jul 31, 2026 at 02:20 A-Shehab <ahshehab24@gmail.com> wrote:

> From: Max Chou <max.chou@sifive.com>
>
> Move gen_update_pc call before conditional logic to ensure consistent
> PC state regardless of execution path.
>
> Previously, the host instructions generated to update the cpu_pc were
> only executed in the failure path when shadow stack validation failed.
> This created inconsistent PC synchronization.
>
> This inconsistency caused issues in CF_PCREL mode where subsequent
> instructions calculated wrong relative offsets from stale pc_save
> values, and could lead to incorrect exception return addresses.
>
> This fix ensures PC is always synchronized before any helper that
> might raise an exception, maintaining consistent translator state
> across all execution paths.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4118
> Signed-off-by: Max Chou <max.chou@sifive.com>
> [ahshehab: rebased on current master; file moved to
>  target/riscv/tcg/insn_trans/ and the ssp load is now 64-bit wide]
> Tested-by: A-Shehab <ahshehab24@gmail.com>
> Signed-off-by: A-Shehab <ahshehab24@gmail.com>
> ---
> This is a repost of Max Chou's patch from 2025-11-05 [1], which did not
> receive any review. Rebased onto current master: the file moved to
> target/riscv/tcg/insn_trans/ and the ssp load is now a 64-bit load, so
> the original patch no longer applies.
>
> I opened a GitLab issue (#4118) with a minimal, self-contained bare-metal
> reproducer for this bug. It runs an sspopchk that matches the shadow stack
> (the common case) followed by an auipc in the same translation block; on
> current master the auipc returns an address 4 bytes too low (exit 42) and
> with this patch it is correct (exit 0). The reproducer is included in the
> issue.
>
> [1]
> https://lore.kernel.org/qemu-devel/20251105134331.2865581-1-max.chou@sifive.com/
>
>  target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> index a813232887..d47a9f9c7d 100644
> --- a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> +++ b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> @@ -32,6 +32,7 @@ static bool trans_sspopchk(DisasContext *ctx,
> arg_sspopchk *a)
>      TCGLabel *skip = gen_new_label();
>      uint32_t tmp = (get_xl(ctx) == MXL_RV64) ? 8 : 4;
>      TCGv data = tcg_temp_new();
> +    gen_update_pc(ctx, 0);
>      TCGv_i64 wide_addr = tcg_temp_new_i64();
>      tcg_gen_ld_i64(wide_addr, tcg_env, offsetof(CPURISCVState, ssp));
>      tcg_gen_trunc_i64_tl(addr, wide_addr);
> @@ -42,7 +43,6 @@ static bool trans_sspopchk(DisasContext *ctx,
> arg_sspopchk *a)
>      tcg_gen_brcond_tl(TCG_COND_EQ, data, rs1, skip);
>      tcg_gen_st8_i32(tcg_constant_i32(RISCV_EXCP_SW_CHECK_BCFI_TVAL),
>                      tcg_env, offsetof(CPURISCVState, sw_check_code));
> -    gen_update_pc(ctx, 0);
>      gen_helper_raise_exception(tcg_env,
>                    tcg_constant_i32(RISCV_EXCP_SW_CHECK));
>      gen_set_label(skip);
> --
> 2.53.0
>
>
Re: [PATCH] target/riscv: Fix PC sync in trans_sspopchk for CFI exception handling
Posted by Ahmed Shehab 1 month, 3 weeks ago 
👍

Ahmed Shehab reacted via Gmail
<https://www.google.com/gmail/about/?utm_source=gmail-in-product&utm_medium=et&utm_campaign=emojireactionemail#app>
Re: [PATCH] target/riscv: Fix PC sync in trans_sspopchk for CFI exception handling
Posted by Daniel Henrique Barboza 1 month, 3 weeks ago 

On 7/30/2026 3:18 PM, A-Shehab wrote:
> From: Max Chou <max.chou@sifive.com>
> 
> Move gen_update_pc call before conditional logic to ensure consistent
> PC state regardless of execution path.
> 
> Previously, the host instructions generated to update the cpu_pc were
> only executed in the failure path when shadow stack validation failed.
> This created inconsistent PC synchronization.
> 
> This inconsistency caused issues in CF_PCREL mode where subsequent
> instructions calculated wrong relative offsets from stale pc_save
> values, and could lead to incorrect exception return addresses.
> 
> This fix ensures PC is always synchronized before any helper that
> might raise an exception, maintaining consistent translator state
> across all execution paths.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/4118
> Signed-off-by: Max Chou <max.chou@sifive.com>
> [ahshehab: rebased on current master; file moved to
>   target/riscv/tcg/insn_trans/ and the ssp load is now 64-bit wide]
> Tested-by: A-Shehab <ahshehab24@gmail.com>
> Signed-off-by: A-Shehab <ahshehab24@gmail.com>
> ---

Reviewed-by: Daniel Henrique Barboza <daniel.barboza@oss.qualcomm.com>

> This is a repost of Max Chou's patch from 2025-11-05 [1], which did not
> receive any review. Rebased onto current master: the file moved to
> target/riscv/tcg/insn_trans/ and the ssp load is now a 64-bit load, so
> the original patch no longer applies.
> 
> I opened a GitLab issue (#4118) with a minimal, self-contained bare-metal
> reproducer for this bug. It runs an sspopchk that matches the shadow stack
> (the common case) followed by an auipc in the same translation block; on
> current master the auipc returns an address 4 bytes too low (exit 42) and
> with this patch it is correct (exit 0). The reproducer is included in the
> issue.
> 
> [1] https://lore.kernel.org/qemu-devel/20251105134331.2865581-1-max.chou@sifive.com/
> 
>   target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> index a813232887..d47a9f9c7d 100644
> --- a/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> +++ b/target/riscv/tcg/insn_trans/trans_rvzicfiss.c.inc
> @@ -32,6 +32,7 @@ static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a)
>       TCGLabel *skip = gen_new_label();
>       uint32_t tmp = (get_xl(ctx) == MXL_RV64) ? 8 : 4;
>       TCGv data = tcg_temp_new();
> +    gen_update_pc(ctx, 0);
>       TCGv_i64 wide_addr = tcg_temp_new_i64();
>       tcg_gen_ld_i64(wide_addr, tcg_env, offsetof(CPURISCVState, ssp));
>       tcg_gen_trunc_i64_tl(addr, wide_addr);
> @@ -42,7 +43,6 @@ static bool trans_sspopchk(DisasContext *ctx, arg_sspopchk *a)
>       tcg_gen_brcond_tl(TCG_COND_EQ, data, rs1, skip);
>       tcg_gen_st8_i32(tcg_constant_i32(RISCV_EXCP_SW_CHECK_BCFI_TVAL),
>                       tcg_env, offsetof(CPURISCVState, sw_check_code));
> -    gen_update_pc(ctx, 0);
>       gen_helper_raise_exception(tcg_env,
>                     tcg_constant_i32(RISCV_EXCP_SW_CHECK));
>       gen_set_label(skip);

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.