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

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

On 24/09/13 15:32, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 13, 2013 at 03:13:30PM +0100, David Vrabel 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.
>>
>> This means that for MFNs that are outside the mapped range of the m2p
>> then mfn_to_pfn() will always look in the m2p overrides.  This is
>> correct because it must be a foreign MFN (and the PFN in the m2p in
>> this case is only relevant for the other domain).
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
>> Cc: Jan Beulich <JBeulich@suse.com>
> 
> This looks like it should also go in stable?

It only really affects 32-bit dom0 and I've generally not been tagging
fixes for this use case for stable.  Your call though.

David