> -----Original Message----- > From: Jan Beulich [mailto:jbeul...@suse.com] > Sent: 11 October 2017 09:31 > To: Paul Durrant <paul.durr...@citrix.com> > Cc: Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap > <george.dun...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; Wei Liu > <wei.l...@citrix.com>; 'Stefano Stabellini' <sstabell...@kernel.org>; 'xen- > de...@lists.xenproject.org' <xen-de...@lists.xenproject.org>; > 'KonradRzeszutek Wilk' <konrad.w...@oracle.com>; Tim (Xen.org) > <t...@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.durr...@citrix.com> wrote: > >> From: Paul Durrant > >> Sent: 10 October 2017 15:10 > >> > From: Jan Beulich [mailto:jbeul...@suse.com] > >> > Sent: 09 October 2017 15:23 > >> > To: Paul Durrant <paul.durr...@citrix.com> > >> > >>> On 06.10.17 at 14:25, <paul.durr...@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
