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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 04/18/2018 10:48 AM, Razvan Cojocaru wrote:
> 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.

Sorry, unlikely(end <= start), obviously.


Thanks,
Razvan

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