Re: [PATCH 2/2] x86/HVM: cache attribute pinning adjustments

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 03/03/16 12:10, Wei Liu wrote:
> On Thu, Mar 03, 2016 at 11:03:43AM +0000, Andrew Cooper wrote:
> [...]
>>> @@ -587,20 +578,21 @@ static void free_pinned_cacheattr_entry(
>>>      xfree(container_of(rcu, struct hvm_mem_pinned_cacheattr_range, rcu));
>>>  }
>>>  
>>> -int32_t hvm_set_mem_pinned_cacheattr(
>>> -    struct domain *d,
>>> -    uint64_t gfn_start,
>>> -    uint64_t gfn_end,
>>> -    uint32_t  type)
>>> +int hvm_set_mem_pinned_cacheattr(struct domain *d, uint64_t gfn_start,
>>> +                                 uint64_t gfn_end, uint32_t type)
>>>  {
>>>      struct hvm_mem_pinned_cacheattr_range *range;
>>>      int rc = 1;
>>>  
>>> -    if ( !is_hvm_domain(d) || gfn_end < gfn_start )
>>> -        return 0;
>>> +    if ( !is_hvm_domain(d) )
>>> +        return -EOPNOTSUPP;
>> You introduce an asymmetry between set and get here, both in terms of
>> the checks (hvm vs hvm_container), and assert vs plain failure.  Why is
>> this?
>>
>> I would suggest ASSERT(is_hvm_domain(d)) in both cases.
>>
> I don't think we can have ASSERT() in the set function because it might
> be called by untrusted entity. On the other hand, the get function can
> only be used by hypervisor so the ASSERT should be fine.

The hypercall handler should sanitise the untrusted caller before we get
into this function.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel