[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v7 10/10] xen/common: do not implicitly permit access to mapped I/O memory

>>> On 26.05.14 at 14:15, <julien.grall@xxxxxxxxxx> wrote:

> On 26/05/14 12:51, Jan Beulich wrote:
>>>>> On 26.05.14 at 13:42, <julien.grall@xxxxxxxxxx> wrote:
>>> On 26/05/14 12:37, Jan Beulich wrote:
>>>>>>> On 26.05.14 at 13:24, <julien.grall@xxxxxxxxxx> wrote:
>>>>> On 26/05/14 12:14, Jan Beulich wrote:
>>>>>>>>> On 26.05.14 at 12:53, <julien.grall@xxxxxxxxxx> wrote:
>>>>>>> On 26/05/14 11:14, Jan Beulich wrote:
>>>>>>>> Or maybe I wasn't wrong - the patch context doesn't really make
>>>>>>>> clear whether it's the granting or mapping operation that gets
>>>>>>>> adjusted here (since an earlier patch moved the mapping one into
>>>>>>>> this function).
>>>>>>>              ret = -EPERM;
>>>>>>> -        if ( !iomem_access_permitted(current->domain, mfn, mfn_end) )
>>>>>>> +        if ( !iomem_access_permitted(d, mfn, mfn_end) )
>>>>>>>                  break;
>>>>>>>              ret = xsm_iomem_mapping(XSM_HOOK, d, mfn, mfn_end, add);
>>>>>>> There is an xsm_iomem_mapping just after, so the change has been done in
>>>>>>> XEN_DOMCTL_memory_mapping.
>>>>>> In which case I indeed stick to my original comment - it's perhaps
>>>>>> best to check _both_.
>>>>> Why? We may want to map the region in the guest P2M without giving the
>>>>> permission to the guest (I'm thinking about ARM passthrough case).
>>>> How can you put a mapping of memory into a guest's P2M for which
>>>> that guest has no access permission? To me this reads like you're
>>>> intending to create a security issue here.
>>> iomem_access_permitted is used to check if we allow the current guest to
>>> map a region in another guest P2M.
>>> Once the mapping is done, at least on ARM, we don't use anymore the
>>> permission check. This is because there is no trap involved afterwards.
>> I don't see how absence or presence of traps is involved here. The
>> problem I see is that by putting in such a P2M entrry you allow a
>> guest access to memory that it wasn't granted access to.
> In the case of an HVM guest (or ARM guest), the permission seems to be 
> used only during DOMCTL_memory_mapping hypercall. So I understand the 
> permission as "I'm allowed to map/unmap this MMIO range from a guest P2M".

Ah, I start seeing where you're coming from: Taking current uses of
a certain construct to imply a meaning of that construct is kind of
backwards. You should start with the meaning, and all of
{iomem,ioports,pirq}_access_permitted() are inquiries to find out
whether the specified domain is permitted to access the specified

And the fact that for HVM/ARM there's currently only that one use
in the domctl handling isn't indicative of anything. Just take the use
in xen/common/grant_table.c - if the paging_mode_external()
check got dropped (assuming the backing functions can handle this)
you'd all of the sudden have another use. And the check here, from
all I know, isn't there because such an operation makes sense only
for PV guests, but because the same operation just can't be handled
for HVM/PVH/ARM ones. The moment someone finds a legitimate use
of e.g. granting pages from a passed-through frame buffer (to e.g.
snapshot its contents directly to storage) this would need to be

> If we request the guest to have the permission on this range, we also 
> allow the guest to map in its P2M (assuming XSM is not there) theses MMIOs.
> I don't think, at least on ARM, we want to let the guest doing what it 
> wants with the mapping MMIO region.

And I didn't say that.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.