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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Mar 03, 2016 at 12:12:40PM +0000, Andrew Cooper wrote:
> 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.
> 

I don't disagree. It is just that this seems undone at the moment -- I
don't see xsm hook in the callsite of this function in domctl.

Wei.

> ~Andrew

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