>>>> +/*
>>>> + * 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. Should I have the
single version call the _multi version after this change?
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
