Re: [Xen-devel] Ping: [Patch v2] x86/mm: Prevent leaking domain mappings in paging_log_dirty_op()

[Tim Deegan <tim@xxxxxxx>]

At 09:14 +0000 on 17 Dec (1387268042), Jan Beulich wrote:
> >>> On 10.12.13 at 15:50, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> > Coverity ID: 1135374 1135375 1135376 1135377
> > 
> > If {copy_to,clear}_guest_offset() fails, we would leak the domain mappings 
> > for
> > l4 thru l1.
> > 
> > Fixing this requires having conditional unmaps on the faulting path, which 
> > in
> > turn requires explicitly initialising the pointers to NULL because of the
> > early ENOMEM exit.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > CC: Keir Fraser <keir@xxxxxxx>
> > Reviewed-by: Jan Beulich <JBeulich@xxxxxxxx>
> > CC: Tim Deegan <tim@xxxxxxx>
> 
> Tim?

Sorry, I hadn't seen v2.  I think I'd prefer the unmap to be done at
the one 'goto out', where we know that all four are mapped (so no
initializers needed either; doing it this way may lead to double-frees
if any more 'goto out's get added.  But taking into account Jan's
comment about dropping the locks first, I guess that's OK.

Acked-by: Tim Deegan <tim@xxxxxxx>

> > ---
> > 
> > Changes in v2:
> >  * Reorder unmaps until after the unlock/unpause (Suggested by Jan)
> > ---
> >  xen/arch/x86/mm/paging.c |   14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> > index 4ba7669..21344e5 100644
> > --- a/xen/arch/x86/mm/paging.c
> > +++ b/xen/arch/x86/mm/paging.c
> > @@ -330,8 +330,8 @@ int paging_log_dirty_op(struct domain *d, struct 
> > xen_domctl_shadow_op *sc)
> >  {
> >      int rv = 0, clean = 0, peek = 1;
> >      unsigned long pages = 0;
> > -    mfn_t *l4, *l3, *l2;
> > -    unsigned long *l1;
> > +    mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
> > +    unsigned long *l1 = NULL;
> >      int i4, i3, i2;
> >  
> >      domain_pause(d);
> > @@ -434,6 +434,16 @@ int paging_log_dirty_op(struct domain *d, struct 
> > xen_domctl_shadow_op *sc)
> >   out:
> >      paging_unlock(d);
> >      domain_unpause(d);
> > +
> > +    if ( l1 )
> > +        unmap_domain_page(l1);
> > +    if ( l2 )
> > +        unmap_domain_page(l2);
> > +    if ( l3 )
> > +        unmap_domain_page(l3);
> > +    if ( l4 )
> > +        unmap_domain_page(l4);
> > +
> >      return rv;
> >  }
> >  
> > -- 
> > 1.7.10.4
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@xxxxxxxxxxxxx 
> > http://lists.xen.org/xen-devel 
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

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