Re: [Xen-devel] [PATCH] x86/p2m: fixed p2m_change_type_range() start / end check

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 04/18/2018 08:38 AM, Jan Beulich wrote:
>>>> On 17.04.18 at 19:16, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -976,6 +976,13 @@ void p2m_change_type_range(struct domain *d,
>>      ASSERT(p2m_is_changeable(ot) && p2m_is_changeable(nt));
>>  
>>      p2m_lock(p2m);
>> +
>> +    if ( start > p2m->max_mapped_pfn )
>> +    {
>> +        p2m_unlock(p2m);
>> +        return;
>> +    }
> 
> I realize this is what George has suggested, but I still wonder if this is the
> right thing to do here: Why is this any more important to check than the
> more general start >= end (in which case the assertion in the rangeset
> code would also trigger)? Till now the function assumes "sensible" things
> to be passed in, but specifically also permitting the [start,~0UL] case
> (just in a more generalizer fashion). The problem you're trying to fix here
> is something passing in a nonsense range (starting above the valid range).
> Yet if we want the function to be immune to nonsense being passed in, I
> think start < end is what needs checking for, and that check would then
> perhaps better go after end was already adjusted.
> 
> The obvious alternative is for callers to only pass in sane ranges (in which
> case adding ASSERT() here may be considered, instead of triggering the
> one in the rangeset code).

The goal of the fix wasn't to get rid of ASSERTs for "nonsense" input
entirely, but to accept input which is in fact "sensible" for altp2m
systems.

I do agree though, that we should either ASSERT for all "nonsense"
input, or handle all such input gracefully.  I'm inclined to ASSERT();
in which case maybe it would be best if we put a bunch up front:

ASSERT(start < end);
if ( start > p2m->max_mapped_pfn)
{
    ASSERT(!p2m_is_hostp2m(p2m));
    ...
}

if ( end > p2m->max_mapped_pfn )
{
    ASSERT(end == ~0UL || !p2m_is_hostp2m(p2m));
    ...

That is, sensible input looks like:
* start < end
* For hostp2ms:
 - start <= max_mapped_pfn
 - either end <= max_mapped_pfn, or end == ~0UL
(But for altp2ms, start or end > max_mapped_pfn is fine.)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel