Re: [PATCH] xen/p2m: check MFN is in range before using the m2p table

[David Vrabel <david.vrabel@citrix.com>]

On 29/08/13 08:07, Jan Beulich wrote:
>>>> On 28.08.13 at 19:30, David Vrabel <david.vrabel@citrix.com> wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> On hosts with more than 168 GB of memory, a 32-bit guest may attempt
>> to grant map an MFN that is not cannot lookup in its mapping of the
>> m2p table.  There is an m2p lookup as part of m2p_add_override() and
>> m2p_remove_override().  The lookup falls off the end of the mapped
>> portion of the m2p and (because the mapping is at the highest virtual
>> address) wraps around and the lookup causes a fault on what appears to
>> be a user space address.
>>
>> do_page_fault() (thinking it's a fault to a userspace address), tries
>> to lock mm->mmap_sem.  If the gntdev device is used for the grant map,
>> m2p_add_override() is called from from gnttab_mmap() with mm->mmap_sem
>> already locked.  do_page_fault() then deadlocks.
>>
>> The deadlock would most commonly occur when a 64-bit guest is started
>> and xenconsoled attempts to grant map its console ring.
>>
>> Introduce mfn_to_pfn_no_overrides() which checks the MFN is within the
>> mapped portion of the m2p table before accessing the table and use
>> this in m2p_add_override(), m2p_remove_override(), and mfn_to_pfn()
>> (which already had the correct range check).
>>
>> All faults caused by accessing the non-existant parts of the m2p are
>> thus within the kernel address space and exception_fixup() is called
>> without trying to lock mm->mmap_sem.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> This all looks quite fine to me, but iiuc it only removes the deadlock,
> it doesn't make things work. In order to make it work I think we'd
> need a hypercall (albeit even then it would work only up to 1Tb).
> Otoh the question of course is whether driving this big a system
> with a 32-bit Dom0 kernel is a reasonable thing in the first place.

It should be sufficient as either an MFN is owned by this domain and
thus it is within the m2p mapping or it's a foreign MFN and it will be
looked up via the m2p overrides.

However, I note I got one of the tests wrong in mfn_to_pfn() and it's
broken.  It should be:

	pfn = mfn_to_pfn_no_overrides(mfn);
	if (pfn == ~0 || get_phys_to_machine(pfn) != mfn)
		pfn = m2p_find_override_pfn(mfn, ~0);

Or the equivalent (because get_phys_to_machine(~0) returns
INVALID_P2M_ENTRY which is != mfn:

	pfn = mfn_to_pfn_no_overrides(mfn);
	if (get_phys_to_machine(pfn) != mfn)
		pfn = m2p_find_override_pfn(mfn, ~0);

David