Re: [Xen-devel] [PATCH 5/5] xen: arm: fix guest register access.

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 18 Dec 2012, Ian Campbell wrote:
> We weren't taking the guest mode (CPSR) into account and would always
> access the user version of the registers.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> ---
>  xen/arch/arm/traps.c       |   62 ++++++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/vgic.c        |    4 +-
>  xen/arch/arm/vpl011.c      |    4 +-
>  xen/arch/arm/vtimer.c      |    8 +++---
>  xen/include/asm-arm/regs.h |    6 ++++
>  5 files changed, 74 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 096dc0b..e3c0290 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -73,6 +73,64 @@ static void print_xen_info(void)
>             debug, print_tainted(taint_str));
>  }
>  
> +uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg)
> +{
> +    BUG_ON( guest_mode(regs) );
> +
> +    /*
> +     * We rely heavily on the layout of cpu_user_regs to avoid having
> +     * to handle all of the registers individually. Use BUILD_BUG_ON to
> +     * ensure that things which expect are contiguous actually are.
> +     */
> +#define REGOFFS(R) offsetof(struct cpu_user_regs, R)
> +
> +    switch ( reg ) {
> +    case 0 ... 7: /* Unbanked registers */
> +        BUILD_BUG_ON(REGOFFS(r0) + 7*sizeof(uint32_t) != REGOFFS(r7));
> +        return &regs->r0 + reg;
> +    case 8 ... 12: /* Register banked in FIQ mode */
> +        BUILD_BUG_ON(REGOFFS(r8_fiq) + 4*sizeof(uint32_t) != 
> REGOFFS(r12_fiq));
> +        if ( fiq_mode(regs) )
> +            return &regs->r8_fiq + reg - 8;
> +        else
> +            return &regs->r8_fiq + reg - 8;

what's the point of this if?


> +    case 13 ... 14: /* Banked SP + LR registers */
> +        BUILD_BUG_ON(REGOFFS(sp_fiq) + 1*sizeof(uint32_t) != 
> REGOFFS(lr_fiq));
> +        BUILD_BUG_ON(REGOFFS(sp_irq) + 1*sizeof(uint32_t) != 
> REGOFFS(lr_irq));
> +        BUILD_BUG_ON(REGOFFS(sp_svc) + 1*sizeof(uint32_t) != 
> REGOFFS(lr_svc));
> +        BUILD_BUG_ON(REGOFFS(sp_abt) + 1*sizeof(uint32_t) != 
> REGOFFS(lr_abt));
> +        BUILD_BUG_ON(REGOFFS(sp_und) + 1*sizeof(uint32_t) != 
> REGOFFS(lr_und));
> +        switch ( regs->cpsr & PSR_MODE_MASK )
> +        {
> +        case PSR_MODE_USR:
> +        case PSR_MODE_SYS: /* Sys regs are the usr regs */
> +            if ( reg == 13 )
> +                return &regs->sp_usr;
> +            else /* lr_usr == lr in a user frame */
> +                return &regs->lr;
> +        case PSR_MODE_FIQ:
> +            return &regs->sp_fiq + reg - 13;
> +        case PSR_MODE_IRQ:
> +            return &regs->sp_irq + reg - 13;
> +        case PSR_MODE_SVC:
> +            return &regs->sp_svc + reg - 13;
> +        case PSR_MODE_ABT:
> +            return &regs->sp_abt + reg - 13;
> +        case PSR_MODE_UND:
> +            return &regs->sp_und + reg - 13;
> +        case PSR_MODE_MON:
> +        case PSR_MODE_HYP:
> +        default:
> +            BUG();
> +        }
> +    case 15: /* PC */
> +        return &regs->pc;
> +    default:
> +        BUG();
> +    }
> +#undef REGOFFS
> +}
> +
>  static const char *decode_fsc(uint32_t fsc, int *level)
>  {
>      const char *msg = NULL;
> @@ -448,7 +506,7 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> unsigned int code)
>      switch ( code ) {
>      case 0xe0 ... 0xef:
>          reg = code - 0xe0;
> -        r = &regs->r0 + reg;
> +        r = select_user_reg(regs, reg);
>          printk("DOM%d: R%d = %#010"PRIx32" at %#010"PRIx32"\n",
>                 domid, reg, *r, regs->pc);
>          break;
> @@ -518,7 +576,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>                         union hsr hsr)
>  {
>      struct hsr_cp32 cp32 = hsr.cp32;
> -    uint32_t *r = &regs->r0 + cp32.reg;
> +    uint32_t *r = select_user_reg(regs, cp32.reg);
>  
>      if ( !cp32.ccvalid ) {
>          dprintk(XENLOG_ERR, "cp_15(32): need to handle invalid condition 
> codes\n");
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 3f7e757..59780d2 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -160,7 +160,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> -    uint32_t *r = &regs->r0 + dabt.reg;
> +    uint32_t *r = select_user_reg(regs, dabt.reg);
>      struct vgic_irq_rank *rank;
>      int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS);
>      int gicd_reg = REG(offset);
> @@ -372,7 +372,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> -    uint32_t *r = &regs->r0 + dabt.reg;
> +    uint32_t *r = select_user_reg(regs, dabt.reg);
>      struct vgic_irq_rank *rank;
>      int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS);
>      int gicd_reg = REG(offset);
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 1522667..7dcee90 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -92,7 +92,7 @@ static int uart0_mmio_read(struct vcpu *v, mmio_info_t 
> *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> -    uint32_t *r = &regs->r0 + dabt.reg;
> +    uint32_t *r = select_user_reg(regs, dabt.reg);
>      int offset = (int)(info->gpa - UART0_START);
>  
>      switch ( offset )
> @@ -114,7 +114,7 @@ static int uart0_mmio_write(struct vcpu *v, mmio_info_t 
> *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
>      struct cpu_user_regs *regs = guest_cpu_user_regs();
> -    uint32_t *r = &regs->r0 + dabt.reg;
> +    uint32_t *r = select_user_reg(regs, dabt.reg);
>      int offset = (int)(info->gpa - UART0_START);
>  
>      switch ( offset )
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 490b021..07994b2 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -21,7 +21,7 @@
>  #include <xen/lib.h>
>  #include <xen/timer.h>
>  #include <xen/sched.h>
> -#include "gic.h"
> +#include <asm/regs.h>
>  
>  extern s_time_t ticks_to_ns(uint64_t ticks);
>  extern uint64_t ns_to_ticks(s_time_t ns);
> @@ -49,7 +49,7 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs, 
> union hsr hsr)
>  {
>      struct vcpu *v = current;
>      struct hsr_cp32 cp32 = hsr.cp32;
> -    uint32_t *r = &regs->r0 + cp32.reg;
> +    uint32_t *r = select_user_reg(regs, cp32.reg);
>      s_time_t now;
>  
>      switch ( hsr.bits & HSR_CP32_REGS_MASK )
> @@ -101,8 +101,8 @@ static int vtimer_emulate_64(struct cpu_user_regs *regs, 
> union hsr hsr)
>  {
>      struct vcpu *v = current;
>      struct hsr_cp64 cp64 = hsr.cp64;
> -    uint32_t *r1 = &regs->r0 + cp64.reg1;
> -    uint32_t *r2 = &regs->r0 + cp64.reg2;
> +    uint32_t *r1 = select_user_reg(regs, cp64.reg1);
> +    uint32_t *r2 = select_user_reg(regs, cp64.reg2);
>      uint64_t ticks;
>      s_time_t now;
>  
> diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
> index 54f6ed8..7486944 100644
> --- a/xen/include/asm-arm/regs.h
> +++ b/xen/include/asm-arm/regs.h
> @@ -30,6 +30,12 @@
>  
>  #define return_reg(v) ((v)->arch.cpu_info->guest_cpu_user_regs.r0)
>  
> +/*
> + * Returns a pointer to the given register value in regs, taking the
> + * processor mode (CPSR) into account.
> + */
> +extern uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg);
> +
>  #endif /* __ARM_REGS_H__ */
>  /*
>   * Local variables:
> -- 
> 1.7.2.5
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel