Xen devel - Apr 2006 - out of bounds handling for get_mfn_from_gpfn()

Since get_mfn_from_gpfn() exclusively relies on recovering from a potential page
fault resulting from an ill gpfn passed
in, I wonder if it is considered the responsibility of the caller to ensure the
gpfn is at least within bounds for
phys_to_machine_mapping[]. We are having a bug report where the function happens
to access (due to way out of bound a
gpfn, the origin of which I have yet to determine) the GDT/LDT space and hence,
instead of recovering, hits the BUG_ON()
in handle_gdt_ldt_mapping_fault().

Assuming it doing the bounds checking is not reasonable to be the responsibility
of the caller, I further wonder in
which of the two possible places the bug should be fixed:
- convert the BUG_ON() in handle_gdt_ldt_mapping_fault() to a conditional,
calling search_exception_table() when VCPU
and area don''t match(and skipping all of the processing and returning
just zero in the opposite case if recovery code
was found), and only BUG()ing when no recovery code can be found
- add a bounds check to get_mfn_from_gpfn() (in which case I''d be
uncertain what the correct boundary is, since on 64
bits (RO_MPT_VIRT_END - RO_MPT_VIRT_START) != (RDWR_MPT_VIRT_END -
RDWR_MPT_VIRT_START), and only one of the two ranges
can be the correct one)

Thanks, Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 26 Apr 2006, at 13:01, Jan Beulich wrote:
> - add a bounds check to get_mfn_from_gpfn() (in which case I''d be 
> uncertain what the correct boundary is, since on 64
> bits (RO_MPT_VIRT_END - RO_MPT_VIRT_START) != (RDWR_MPT_VIRT_END - 
> RDWR_MPT_VIRT_START), and only one of the two ranges
> can be the correct one)
A range check is needed as the function can be passed unvalidated 
values from a guest.

The tables you mention above are not involved in get_mfn_from_gpfn() -- 
they translate the other way. The input gpfn either needs testing 
against, or masking with, (PADDR_MASK >> PAGE_SHIFT). We should always 
ensure that the m2p and p2m tables are big enough to be indexed by that 
value.

I don''t think that the mfn-to-gpfn direction needs a check, but an 
assertion might be worthwhile.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>The tables you mention above are not involved in get_mfn_from_gpfn() -- 
>they translate the other way.
???
xen/include/asm-x86/mm.h says:

/*
 * The phys_to_machine_mapping is the reversed mapping of MPT for full
 * virtualization.  It is only used by shadow_mode_translate()==true
 * guests, so we steal the address space that would have normally
 * been used by the read-only MPT map.
 */
#define phys_to_machine_mapping ((unsigned long *)RO_MPT_VIRT_START)
#define INVALID_MFN             (~0UL)
#define VALID_MFN(_mfn)         (!((_mfn) & (1U<<31)))

#define set_mfn_from_gpfn(pfn, mfn) (phys_to_machine_mapping[(pfn)] = (mfn))
static inline unsigned long get_mfn_from_gpfn(unsigned long pfn)
{
    unsigned long mfn;

    if ( __copy_from_user(&mfn, &phys_to_machine_mapping[pfn],
sizeof(mfn)) )
	mfn = INVALID_MFN;

    return mfn;
}
>The input gpfn either needs testing 
>against, or masking with, (PADDR_MASK >> PAGE_SHIFT). We should always
>ensure that the m2p and p2m tables are big enough to be indexed by that 
>value.
>From the above header excerpt I''m getting the impression that mfn-s
are assumed to be at most 31 bits in size (bit 31
is used as an invalid indicator there). Also, in the observed case the mfn is a
39-bit value, which is clearly inside
the range covered by (PADDR_MASK >> PAGE_SHIFT) (40 bits), however,
(RO_MPT_VIRT_END - RO_MPT_VIRT_START) covers only 38
bits (where, for the full 40-bit MFN range, 43 bits would be needed, which would
cover the entire current hypervisor
address space reservation).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 26 Apr 2006, at 14:21, Jan Beulich wrote:
>> The tables you mention above are not involved in get_mfn_from_gpfn() 
>> --
>> they translate the other way.
>
> ???
> xen/include/asm-x86/mm.h says:
Ah, yes, p2m table actually steals the read-only m2p memory area for 
translated guests. So it''s that area you''re interested in (not
the
RW_MPT, which really is always the m2p table).
>
>> The input gpfn either needs testing
>> against, or masking with, (PADDR_MASK >> PAGE_SHIFT). We should
always
>> ensure that the m2p and p2m tables are big enough to be indexed by 
>> that
>> value.
>
> From the above header excerpt I''m getting the impression that
mfn-s
> are assumed to be at most 31 bits in size (bit 31
> is used as an invalid indicator there). Also, in the observed case the 
> mfn is a 39-bit value, which is clearly inside
> the range covered by (PADDR_MASK >> PAGE_SHIFT) (40 bits), however, 
> (RO_MPT_VIRT_END - RO_MPT_VIRT_START) covers only 38
> bits (where, for the full 40-bit MFN range, 43 bits would be needed, 
> which would cover the entire current hypervisor
> address space reservation).
Okay, maybe we need to be more restrictive: 20 bits for 32b (4GB), 22 
bits for 32b-pae (16GB), and some suitable large value for 64b (31 
would be fine, it''s good for up to 8TB).

It''d be nice to set PADDR_BITS appropriately and use a value derived 
from that, but the currently-chosen (larger) values are important for 
pte checking. If we exclude bits from the pte''s pfn check by making 
PADDR_BITS smaller then we''ll have to deal with the high-order pte bits
some other way (perhaps by folding them into the value returned by 
e.g., l1e_get_flags()).

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>Okay, maybe we need to be more restrictive: 20 bits for 32b (4GB), 22 
>bits for 32b-pae (16GB), and some suitable large value for 64b (31 
>would be fine, it''s good for up to 8TB).
Then perhaps the way to go would be to really check against
((RO_MPT_VIRT_END - RO_MPT_VIRT_START) / sizeof(*phys_to_machine_mapping)).
This wouldn''t be 31 bits for x86-64, but it seems to me that allowing
35-bit gpfn-s
wouldn''t be that bad an idea (and the fact that the array currently may
only get
populated up to 31-bit gpfn-s wouldn''t seem to cause any problems).
I''ll prepare a
patch...

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 26 Apr 2006, at 15:28, Jan Beulich wrote:
>> Okay, maybe we need to be more restrictive: 20 bits for 32b (4GB), 22
>> bits for 32b-pae (16GB), and some suitable large value for 64b (31
>> would be fine, it''s good for up to 8TB).
>
> Then perhaps the way to go would be to really check against
> ((RO_MPT_VIRT_END - RO_MPT_VIRT_START) / 
> sizeof(*phys_to_machine_mapping)).
> This wouldn''t be 31 bits for x86-64, but it seems to me that
allowing
> 35-bit gpfn-s
> wouldn''t be that bad an idea (and the fact that the array
currently
> may only get
> populated up to 31-bit gpfn-s wouldn''t seem to cause any
problems).
> I''ll prepare a
> patch...
Okay, that''s a reasonable way to go. Please define a macro to hide the 
RO_MPT arithmetic though.
Maybe NR_PHYSMACHINE_TABLE_ENTRIES or something like that...

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel