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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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
resource.

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
fixed.

> 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.

Jan


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