On 12/24/19 9:04 AM, Alexandru Stefan ISAILA wrote:
>>>>> +/*
>>>>> + * Set/clear the #VE suppress bit for multiple pages. Only available on
>>>>> VMX.
>>>>> + */
>>>>> +int p2m_set_suppress_ve_multi(struct domain *d,
>>>>> + struct xen_hvm_altp2m_suppress_ve_multi
>>>>> *sve)
>>>>> +{
>>>>> + struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>>>>> + struct p2m_domain *ap2m = NULL;
>>>>> + struct p2m_domain *p2m = host_p2m;
>>>>> + uint64_t start = sve->first_gfn;
>>>>> + int rc = 0;
>>>>> +
>>>>> + if ( sve->view > 0 )
>>>>> + {
>>>>> + if ( sve->view >= MAX_ALTP2M ||
>>>>> + d->arch.altp2m_eptp[array_index_nospec(sve->view,
>>>>> MAX_ALTP2M)] ==
>>>>> + mfn_x(INVALID_MFN) )
>>>>> + return -EINVAL;
>>>>> +
>>>>> + p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(sve->view,
>>>>> + MAX_ALTP2M)];
>>>>> + }
>>>>> +
>>>>> + p2m_lock(host_p2m);
>>>>> +
>>>>> + if ( ap2m )
>>>>> + p2m_lock(ap2m);
>>>>> +
>>>>> + while ( sve->last_gfn >= start )
>>>>> + {
>>>>> + p2m_access_t a;
>>>>> + p2m_type_t t;
>>>>> + mfn_t mfn;
>>>>> + int err = 0;
>>>>> +
>>>>> + if ( altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a,
>>>>> AP2MGET_query) )
>>>>> + a = p2m->default_access;
>>>>
>>>> So in the single-entry version, if altp2m_get_effective_entry() returns
>>>> an error, you pass that error up the stack; but in the multiple-entry
>>>> version, you ignore the error and simply set the access to
>>>> default_access? I don't think that can be right. If it is right, then
>>>> it definitely needs a comment.
>>>>
>>>
>>> The idea behind this was to have a best effort try and signal the first
>>> error. If the get_entry fails then the best way to go is with
>>> default_access but this is open for debate.
>>
>> I don't see how it's a good idea at all. If get_effective_entry fails,
>> then mfn and t may both be uninitialized. If an attacker can arrange
>> for those to have the values she wants, she could use this to take over
>> the system.
>>
>>> Another way to solve this is to update the first_error_gfn/first_error
>>> and then continue. I think this ca be used to make p2m_set_suppress_ve()
>>> call p2m_set_suppress_ve_multi.
>>
>> Isn't that exactly the semantics you want -- try gfn N, if that fails,
>> record it and move on to the next one? Why would "write an entry with
>> random values for mfn and type, but with the default access" be a better
>> response?
>>
>
> That is right, I'll go with this for the next version.
So, one potential behavior you might want. Consider the following case:
gfn 'A' isn't mapped in the hostp2m yet.
1. Create altp2m X
2. Tools set the sve gfn A
3. Host adds mapping for A
4. Guest accesses A, faulting the mapping over to the altp2m
At the moment, for the single-entry call, #2 will fail, and #4 will get
the default sve value. It might be nice for #2 to succeed, and #4 to
copy over the mfn, type, &c, but use the sve value specified in #2.
But at the moment, altp2m_get_or_propagate() won't end up copying sve
over if the altp2m entry is invalid (AFAICT). So I think for now,
skipping that entry and leaving it an error is the best thing to do.
> Should I have the
> single version call the _multi version after this change?
That seems like a good thing to try. Thanks.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
