Re: [Xen-devel] [PATCH 01/13] xen/arm: domain: Zeroed the vCPU stack

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 25/05/18 21:52, Stefano Stabellini wrote:
> On Tue, 22 May 2018, Julien Grall wrote:
> > A stack is allocated per vCPU to be used by Xen. The allocation is done
> > with alloc_xenheap_pages that does not zero the memory returned. However
> > the top of the stack is containing information that will be used to
> > store the initial state of the vCPU (see struct cpu_info). Some of the
> > fields may not be initialized and will lead to use/leak bits of previous
> > memory in some cases on the first run of vCPU (AFAICT this only happen on
> > vCPU0 for Dom0).
> >
> > While this is not strictly necessary, this patch zero the full stack to
> > avoid more leakage.
> Well spotted! struct cpu_info is the only instance of these cases, I
> suggest to zero only sizeof(struct cpu_info) to avoid having any impact
> on the boot time.
I really don't believe the impact is noticeable when you look at the 
rest of the domain creation.
> After all, with this series we have the mitigation enabled all the time
> in Xen for XSA-263. Or do you think there are other reasons to be
> concerned?
This has nothing to do with XSA-263. This is more that it would be a 
good practice to zero anything by default rather than relying on the 
code to do the proper initialization. In the case of the stack it would 
be un-initialized value over code called by a domain or even assembly.
We already do that for all Domain specific structure but the stack.

I don't really particularly care to fully zeroed the stack if you don't 
want to see it.
Cheers,

> > This is part of XSA-263.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > ---
> >   xen/arch/arm/domain.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index ec0f042bf7..e7b33e92fb 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -540,6 +540,7 @@ void free_vcpu_struct(struct vcpu *v)
> >   int vcpu_initialise(struct vcpu *v)
> >   {
> >       int rc = 0;
> > +    unsigned int i;
> >   
> >       BUILD_BUG_ON( sizeof(struct cpu_info) > STACK_SIZE );
> >   
> > @@ -547,6 +548,9 @@ int vcpu_initialise(struct vcpu *v)
> >       if ( v->arch.stack == NULL )
> >           return -ENOMEM;
> >   
> > +    for ( i = 0; i < (1U << STACK_ORDER); i++ )
> > +        clear_page(v->arch.stack + (PAGE_SIZE * i));
> > +
> >       v->arch.cpu_info = (struct cpu_info *)(v->arch.stack
> >                                              + STACK_SIZE
> >                                              - sizeof(struct cpu_info));
> > --
> > 2.11.0
--
Julien Grall

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