Re: [PATCH v9 05/11] x86/mm: add HYPERVISOR_memory_op to acquire guest resources

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 11 October 2017 09:31
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'xen-
> devel@lists.xenproject.org' <xen-devel@lists.xenproject.org>;
> 'KonradRzeszutek Wilk' <konrad.wilk@oracle.com>; Tim (Xen.org)
> <tim@xen.org>
> Subject: RE: [PATCH v9 05/11] x86/mm: add HYPERVISOR_memory_op to
> acquire guest resources
> 
> >>> On 10.10.17 at 16:37, <Paul.Durrant@citrix.com> wrote:
> >> From: Paul Durrant
> >> Sent: 10 October 2017 15:10
> >> > From: Jan Beulich [mailto:JBeulich@suse.com]
> >> > Sent: 09 October 2017 15:23
> >> > To: Paul Durrant <Paul.Durrant@citrix.com>
> >> > >>> On 06.10.17 at 14:25, <paul.durrant@citrix.com> wrote:
> >> > > --- a/xen/common/memory.c
> >> > > +++ b/xen/common/memory.c
> >> > > @@ -965,6 +965,67 @@ static long xatp_permission_check(struct
> domain
> >> > *d, unsigned int space)
> >> > >      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
> >> > >  }
> >> > >
> >> > > +#ifdef CONFIG_X86
> >> > > +static int acquire_resource(const xen_mem_acquire_resource_t
> *xmar)
> >> > > +{
> >> > > +    struct domain *d, *currd = current->domain;
> >> > > +    unsigned long mfn_list[2];
> >> > > +    int rc;
> >> > > +
> >> > > +    if ( xmar->nr_frames == 0 || xmar->pad != 0 )
> >> > > +        return -EINVAL;
> >> > > +
> >> > > +    if ( xmar->nr_frames > ARRAY_SIZE(mfn_list) )
> >> > > +        return -E2BIG;
> >> > > +
> >> > > +    d = rcu_lock_domain_by_any_id(xmar->domid);
> >> > > +    if ( d == NULL )
> >> > > +        return -ESRCH;
> >> > > +
> >> > > +    rc = xsm_domain_memory_map(XSM_TARGET, d);
> >> >
> >> > Looking at the description of patch 6 - why is this XSM_TARGET
> >> > rather than XSM_DM_PRIV?
> >>
> >> Good point. I was using the priv mapping code as a guide, but
> XSM_DM_PRIV
> >> is probably the right thing to use in this case.
> >>
> >
> > Actually that's not possible. There is an assertion in
> > xsm_domain_memory_map() that the action is XSM_TARGET.
> 
> Well, I was afraid of this being the case, but this only complicates
> your job, it doesn't make XSM_TARGET the right choice here. But
> wait, maybe it can be considered sufficient, but then this needs
> to be prominently pointed out by a comment added at a suitable
> place: For the ioreq pages, them being owned by the emulating
> domain, page ownership validations while trying to make use of the
> MFNs would prevent mis-use by the domain the emulation is being
> done for. And for grant table pages the guest is able to access
> them another way anyway.
> 
> Which basically leaves the question of this being an information
> leak for ioreq pages, as the guest is not supposed to know the
> MFNs, but could obtain them here. I for one would consider such
> a leak a security issue, even if knowledge of the MFNs alone is
> not enough to exploit anything, but maybe others think differently.
> 

I agree with you. Now you point it out, it does rather defeat the purpose of having the separate resource map hypercall.

> But the grant table aspect suggests anyway that perhaps the
> permission to be checked here needs to depend on resource type.

Separate permissions could be an option, but maybe it would be better just to introduce a new resource mapping permission. I'll probably go with the latter.

  Paul

> 
> Jan


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