Re: [PATCH for-4.6] xen/mm: populate_physmap: validate correctly the gfn for direct mapped domain

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-08-13 at 03:54 -0600, Jan Beulich wrote:
> > 
> > > > On 13.08.15 at 11:33, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-08-11 at 18:41 +0100, Julien Grall wrote:
> > > --- a/xen/common/memory.c
> > > +++ b/xen/common/memory.c
> > > @@ -126,22 +126,28 @@ static void populate_physmap(struct memop_args 
> > > *a)
> > >              if ( is_domain_direct_mapped(d) )
> > >              {
> > >                  mfn = gpfn;
> > > -                if ( !mfn_valid(mfn) )
> > > +
> > > +                for ( j = 0; j < (1 << a->extent_order); j++, mfn++ 
> > > )
> > >                  {
> > > -                    gdprintk(XENLOG_INFO, "Invalid mfn 
> > > %#"PRI_xen_pfn"\n",
> > > +                    if ( !mfn_valid(mfn) )
> > > +                    {
> > > +                        gdprintk(XENLOG_INFO, "Invalid mfn 
> > > %#"PRI_xen_pfn"\n",
> > >                               mfn);
> > > -                    goto out;
> > > +                        goto out;
> > > +                    }
> > > +
> > > +                    page = mfn_to_page(mfn);
> > > +                    if ( !get_page(page, d) )
> > > +                    {
> > > +                        gdprintk(XENLOG_INFO,
> > > +                                 "mfn %#"PRI_xen_pfn" doesn't belong 
> > > to the"
> > > +                                 " domain\n", mfn);
> > > +                        goto out;
> > 
> > This will leak the references on any earlier pages, which will happen 
> > all
> > the time if you are cross a boundary from RAM into e.g. MMIO or 
> > whatever.
> 
> I don't see why it would - the put_page() (as you say a few lines
> down in your reply) follows right afterwards.

Right, sorry I should have delete all this once I realised what was going
on (I wasn't 100% sure I guess).

> > Oh, am I wrong and we do get_page then immediately put_page for every page?
> > I suppose that is one way to check the current owner, but I think it needs
> > a comment at least (it did before TBH, just iterating over many pages makes
> > it seem even odder).
> > 
> > Is that really the best way to ask if a page is owned by a given domain tho
> > ugh? Does page_get_owner not suffice?
> 
> That could produce wrong results when the page previously had
> no reference (and namely, due to the unions used in
> struct page_info, was free or in use as a shadow page).

I see.

Maybe some new helper to encapsulate the get+put to validate the presence
of a current owner would be nice in the future then, but not a blocker for
this patch I guess.

There isn't a race here is there? What if the reference were dropped after
this check but before the guest_physmap_add_page (which takes new
references)? We are implicitly relying on a guarantee made elsewhere that a
direct mapped guest never drops the final ref until it is destroyed (which
can't be happening in this window I think), but it would be obviously safer
against such bugs if we just held the ref until after the p2m was updated.

Another thing for a future/4.7 cleanup I guess.

Ian.