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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/18/2018 10: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).
> 
> In any event you want to add unlikely(), just like the similar end check has.

FWIW I'll be happy to change the test to unlikely( start < end ), if
this is OK with George. I'll test the change now.


Thanks,
Razvan

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