Re: [Xen-devel] [PATCH V3] x86/altp2m: Hypercall to set altp2m view visibility

[Alexandru Stefan ISAILA <aisaila@bitdefender.com>]

On 19.02.2020 19:00, Jan Beulich wrote:
> On 19.02.2020 10:18, Alexandru Stefan ISAILA wrote:
>> @@ -4835,6 +4836,23 @@ static int do_altp2m_op(
>>           break;
>>       }
>>   
>> +    case HVMOP_altp2m_set_visibility:
>> +    {
>> +        uint16_t altp2m_idx = a.u.set_visibility.altp2m_idx;
>> +
>> +        if ( a.u.set_visibility.pad )
>> +            rc = -EINVAL;
>> +        else if ( !altp2m_active(d) )
>> +            rc = -EOPNOTSUPP;
>> +        else if ( a.u.set_visibility.visible )
>> +            d->arch.altp2m_working_eptp[altp2m_idx] =
>> +                d->arch.altp2m_eptp[altp2m_idx];
>> +        else
>> +            d->arch.altp2m_working_eptp[altp2m_idx] =
>> +                mfn_x(INVALID_MFN);
> 
> Don't you need to bounds check the index before its use?

Unless we want a index out of bounds from the user. Sorry for not having 
that, I will add a "altp2m_eptp[array_index_nospec(altp2m_idx, 
MAX_EPTP)]" in place for the next version.

> And
> shouldn't you return an error also for in-range ones which
> aren't actually valid?
That is a good thing. Maybe -EINVAL could fit this?

And together with the bounds check it will end up something like this:

if ( !altp2m_idx < 0 || altp2m_idx >= 
min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) || 
altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] == 
mfn_x(INVALID_MFN) )

return -EINVAL;

And looking again, it could be coupled with the a.u.set_visibility.pad.

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