Re: [Xen-devel] [PATCH] xen: arm: inject unhandled instruction and data aborts to the guest.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-09 at 15:57 +0000, Julien Grall wrote:
> On 12/09/2013 02:58 PM, Ian Campbell wrote:
> > Currently an unhandled data abort in guest context leads to us killing the
> > guest and an unhandled instruction abort in guest context leads to us 
> > killing
> > the host!
> > 
> > Andre pointed out that an unhandled data abort can be caused by e.g. 
> > dmidecode
> > looking for things which are not there in the guests physical address space.
> > Propagating the fault to the guest allows it to properly SIGSEGV the
> > processes.
> > 
> > A guest kernel can trivially jump to an unmapped physical address which 
> > would
> > cause an instruction abort. Killing the host for that is obviously bad.
> > Instead inject the exception so the guest kernel can SIGSEGV or panic() etc 
> > as
> > it deems appropriate.
> > 
> > Tested on arm64 (Mustang) and arm32 (Midway) with a dom0 kernel 
> > late_initcall
> > which either dereferences or jumps to address 0, provoking both behaviours 
> > and
> > resulting correctly in a guest kernel panic. Also tested on fast models 
> > with a
> > 32-bit dom0 on a 64-bit hypervisor, which behaved correctly.
> > 
> > In addition tested on both platforms with a userspace program which either
> > calls to or dereferences address 0. The process is correctly killed with 
> > SEGV.
> > 
> > Lastly tested on Mustang with a 32-bit version of the userspace test on a
> > 64-bit dom0 kernel.
> > 
> > I think that covers all the cases.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: Andre Przywara <andre.przywara@xxxxxxxxxxx>
> > ---
> > Release wise this is a(n important) bug fix.
> > ---
> >  xen/arch/arm/traps.c            |  199 
> > +++++++++++++++++++++++++++++++++------
> >  xen/include/asm-arm/cpregs.h    |    1 +
> >  xen/include/asm-arm/processor.h |   15 ++-
> >  xen/include/asm-arm/regs.h      |    2 +
> >  4 files changed, 184 insertions(+), 33 deletions(-)
> > 
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 2a85d37..e4e7f83 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -264,6 +264,8 @@ static void cpsr_switch_mode(struct cpu_user_regs 
> > *regs, int mode)
> >  
> >      regs->cpsr |= mode;
> >      regs->cpsr |= PSR_IRQ_MASK;
> > +    if (mode == PSR_MODE_ABT)
> 
> if ( ... )

Wow, if that turns out to be the only thing wrong with a patch of this
scope then I'll have done well!

This whole function is wrong in this regard, I'm in two minds about
leaving it as is, but it's only a couple of lines so I suppose I may as
well fix it.

> The patch looks good to me:
> Acked-by: Julien Grall <julien.grall@xxxxxxxxxx>

Thanks.

Ian.


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