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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.08.15 at 11:33, <ian.campbell@xxxxxxxxxx> 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.

> A variant of get_page which took an order argument and returned either with
> 0 or 1<<order refs taken would perhaps make this easier to deal with. I
> suppose we don't already have one of those, but you could add it.

That would be useful if we wanted to retain the ref, but that's not the
case here.

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

Jan


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