Re: [Xen-devel] [PATCH] xen/arm: traps: Avoid using BUG_ON() to check guest state in advance_pc()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 15 May 2019, Julien Grall wrote:
> The condition of the BUG_ON() in advance_pc() is pretty wrong because
> the bits [26:25] and [15:10] have a different meaning between AArch32
> and AArch64 state.
> 
> On AArch32, they are used to store PSTATE.IT. On AArch64, they are RES0
> or used for new feature (e.g ARMv8.0-SSBS, ARMv8.5-BTI).
> 
> This means a 64-bit guest will hit the BUG_ON() if it is trying to use
> any of these features.
> 
> More generally, RES0 means that the bits is reserved for future use. So
> crashing the host is definitely not the right solution.
> 
> In this particular case, we only need to know the guest was using 32-bit
> Mode and the Thumb instructions. So replace the BUG_ON() by a proper
> check.
> 
> Reported-by: Lukas Jünger <lukas.juenger@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

> ---
>     This patch needs to be backported as far as possible. Otherwise Xen
>     would not be able to run on processor implementing ARMv8.0-SSBS,
>     ARMv8.5-BTI or ARMv8.5-MemTag. The former is actually the most
>     critical as this is used for controlling mitagion for SSBD (aka
>     Spectre v4) in hardware.
> ---
>  xen/arch/arm/traps.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index d8b9a8a0f0..798a3a45a4 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1650,12 +1650,9 @@ int check_conditional_instr(struct cpu_user_regs 
> *regs, const union hsr hsr)
>  void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
>  {
>      unsigned long itbits, cond, cpsr = regs->cpsr;
> +    bool is_thumb = psr_mode_is_32bit(cpsr) && (cpsr & PSR_THUMB);
>  
> -    /* PSR_IT_MASK bits can only be set for 32-bit processors in Thumb mode. 
> */
> -    BUG_ON( (!psr_mode_is_32bit(cpsr)||!(cpsr&PSR_THUMB))
> -            && (cpsr&PSR_IT_MASK) );
> -
> -    if ( cpsr&PSR_IT_MASK )
> +    if ( is_thumb && (cpsr & PSR_IT_MASK) )
>      {
>          /* The ITSTATE[7:0] block is contained in CPSR[15:10],CPSR[26:25]
>           *
> -- 
> 2.11.0
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel