[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.