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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 26.05.14 at 13:42, <julien.grall@linaro.org> wrote:

> 
> On 26/05/14 12:37, Jan Beulich wrote:
>>>>> On 26.05.14 at 13:24, <julien.grall@linaro.org> wrote:
>>> On 26/05/14 12:14, Jan Beulich wrote:
>>>>>>> On 26.05.14 at 12:53, <julien.grall@linaro.org> 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.

>>> With your requirements, we have to call 2 hypercalls rather than one for
>>> memory mapping, even if we don't want to allow the guest modifying iomem
>>> range.
>>
>> While I can see you not allowing modification, even r/o access may
>> (and likely will) be problematic for MMIO.
> 
> AFAIU, iomem_access_permitted is only here to allow modification of this 
> range via hypercall.

I don't think I understand what you're trying to tell me here.

Jan