On 12/24/19 8:48 AM, Alexandru Stefan ISAILA wrote:
>
>
> On 24.12.2019 10:30, George Dunlap wrote:
>> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>>> By default the sve bits are not set.
>>> This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
>>> to set a range of sve bits.
>>> The core function, p2m_set_suppress_ve_multi(), does not brake in case
>>
>> *break
>
> Sorry for the typo.
>
>>
>>> of a error and it is doing a best effort for setting the bits in the
>>> given range. A check for continuation is made in order to have
>>> preemption on big ranges.
>>
>> Weird English quirk: this should be "large". ("Big" and "large" are
>> both adjectives, and "ranges" is a noun, so theoretically it should be
>> OK; but if you ask almost any native English speaker they'll say that
>> "big" sounds wrong in this case. No real idea why.)
>>
>> Both of these could be fixed on check-in.
>>
>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>> index 4fc919a9c5..de832dcc6d 100644
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -3070,6 +3070,70 @@ out:
>>> return rc;
>>> }
>>>
>>> +/*
>>> + * 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?
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
