Re: [PATCH] x86/hvm: Fix double free from vlapic_init() early error path

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Mar 31, 2021 at 02:56:27PM +0100, Andrew Cooper wrote:
> On 31/03/2021 14:49, Roger Pau Monné wrote:
> > On Wed, Mar 31, 2021 at 02:31:25PM +0100, Andrew Cooper wrote:
> >> vlapic_init()'s caller calls vlapic_destroy() on error.  Therefore, the 
> >> error
> >> path from __map_domain_page_global() failing would doubly free
> >> vlapic->regs_page.
> >>
> >> Rework vlapic_destroy() to be properly idempotent, introducing the 
> >> necessary
> >> UNMAP_DOMAIN_PAGE_GLOBAL() and FREE_DOMHEAP_PAGE() wrappers.
> >>
> >> Rearrange vlapic_init() to group all trivial initialisation, and leave all
> >> cleanup to the caller, in line with our longer term plans.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> CC: Wei Liu <wl@xxxxxxx>
> >> ---
> >>  xen/arch/x86/hvm/vlapic.c     | 11 ++++-------
> >>  xen/include/xen/domain_page.h |  5 +++++
> >>  xen/include/xen/mm.h          |  6 ++++++
> >>  3 files changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> >> index 5e21fb4937..da030f41b5 100644
> >> --- a/xen/arch/x86/hvm/vlapic.c
> >> +++ b/xen/arch/x86/hvm/vlapic.c
> >> @@ -1608,7 +1608,9 @@ int vlapic_init(struct vcpu *v)
> >>          return 0;
> >>      }
> >>  
> >> +    /* Trivial init. */
> >>      vlapic->pt.source = PTSRC_lapic;
> >> +    spin_lock_init(&vlapic->esr_lock);
> >>  
> >>      vlapic->regs_page = alloc_domheap_page(v->domain, MEMF_no_owner);
> >>      if ( !vlapic->regs_page )
> >> @@ -1616,17 +1618,12 @@ int vlapic_init(struct vcpu *v)
> >>  
> >>      vlapic->regs = __map_domain_page_global(vlapic->regs_page);
> >>      if ( vlapic->regs == NULL )
> >> -    {
> >> -        free_domheap_page(vlapic->regs_page);
> >>          return -ENOMEM;
> >> -    }
> >>  
> >>      clear_page(vlapic->regs);
> >>  
> >>      vlapic_reset(vlapic);
> >>  
> >> -    spin_lock_init(&vlapic->esr_lock);
> >> -
> >>      tasklet_init(&vlapic->init_sipi.tasklet, vlapic_init_sipi_action, v);
> >>  
> >>      if ( v->vcpu_id == 0 )
> >> @@ -1645,8 +1642,8 @@ void vlapic_destroy(struct vcpu *v)
> >>      tasklet_kill(&vlapic->init_sipi.tasklet);
> >>      TRACE_0D(TRC_HVM_EMUL_LAPIC_STOP_TIMER);
> >>      destroy_periodic_time(&vlapic->pt);
> >> -    unmap_domain_page_global(vlapic->regs);
> >> -    free_domheap_page(vlapic->regs_page);
> >> +    UNMAP_DOMAIN_PAGE_GLOBAL(vlapic->regs);
> > I think you need to check whether vlapic->regs_page is NULL here...
> >
> >> +    FREE_DOMHEAP_PAGE(vlapic->regs_page);
> >>  }
> >>  
> >>  /*
> >> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
> >> index a182d33b67..0cb7f2aad3 100644
> >> --- a/xen/include/xen/domain_page.h
> >> +++ b/xen/include/xen/domain_page.h
> >> @@ -77,4 +77,9 @@ static inline void unmap_domain_page_global(const void 
> >> *va) {};
> >>      (p) = NULL;                     \
> >>  } while ( false )
> >>  
> >> +#define UNMAP_DOMAIN_PAGE_GLOBAL(p) do {   \
> >> +    unmap_domain_page_global(p);           \
> >> +    (p) = NULL;                            \
> >> +} while ( false )
> >> +
> >>  #endif /* __XEN_DOMAIN_PAGE_H__ */
> >> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> >> index 667f9dac83..c274e2eac4 100644
> >> --- a/xen/include/xen/mm.h
> >> +++ b/xen/include/xen/mm.h
> >> @@ -85,6 +85,12 @@ bool scrub_free_pages(void);
> >>  } while ( false )
> >>  #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
> >>  
> >> +#define FREE_DOMHEAP_PAGES(p, o) do { \
> >> +    free_domheap_pages(p, o);         \
> > ...as both unmap_domain_page_global and free_domheap_pages don't
> > support being passed a NULL pointer.
> >
> > Passing such NULL pointer will result in unmap_domain_page_global
> > hitting an assert AFAICT, and free_domheap_pages will try to use
> > page_get_owner(NULL).
> 
> Urgh - very good points.
> 
> Do we perhaps want to take the opportunity to make these functions
> tolerate NULL, to simplify all cleanup code across the hypervisor?

Yes please, I prefer that rather than open coding the check in
FREE_DOMHEAP_PAGES/UNMAP_DOMAIN_PAGE_GLOBAL (or the callers).

Thanks, Roger.