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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.17 at 16:37, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Paul Durrant
>> Sent: 10 October 2017 15:10
>> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> > Sent: 09 October 2017 15:23
>> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
>> > >>> On 06.10.17 at 14:25, <paul.durrant@xxxxxxxxxx> 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.

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

Jan


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