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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 29 May 2018, Julien Grall wrote:
> 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.

I'd choose to only zero what we need.


> 
> > > 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