Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages

[Julien Grall <julien.grall@linaro.org>]

On 12/03/15 14:13, Tamas K Lengyel wrote:
> 
> 
> On Thu, Mar 12, 2015 at 2:50 PM, Julien Grall <julien.grall@linaro.org
> <mailto:julien.grall@linaro.org>> wrote:
> 
>     Hi Tamas,
> 
>     On 06/03/15 21:24, Tamas K Lengyel wrote:
>     > +/*
>     > + * If mem_access is in use it might have been the reason why get_page_from_gva
>     > + * failed to fetch the page, as it uses the MMU for the permission checking.
>     > + * Only in these cases we do a software-based type check and fetch the page if
>     > + * we indeed found a conflicting mem_access setting.
>     > + */
>     > +static int check_type_get_page(vaddr_t gva, unsigned long flag,
>     > +                               struct page_info** page)
>     > +{
>     > +    long rc;
>     > +    paddr_t ipa;
>     > +    unsigned long maddr;
>     > +    unsigned long mfn;
>     > +    xenmem_access_t xma;
>     > +    p2m_type_t t;
>     > +
>     > +    rc = gva_to_ipa(gva, &ipa);
> 
>     I though a bit more about this call.
> 
>     gva_to_ipa only checks if the mapping has read-permission. That would
>     allow a guest to write on read-only mapping.
> 
> 
>     You have to pass the flags to gva_to_ipa in order to avoid
>     re-introducing XSA-98 [1]
> 
> 
> Here I really just care if the mapping exist to see if we have a
> mem_access restriction, r/w permission checking is then performed
> afterwards by checking the page type. If there are additional
> restrictions on the page beside the type, those certainly should be
> added. Can you point me to where that additional restriction is stored
> so I can query for it?

The R/W permission checking done afterwards is not enough.

What does get_page_from_gva is:
	1) Check the permission on Stage 1 page table (controlled by the guest
and translate VA -> IPA)
	2) Check the permission on Stage 2 page table (controlled by the
hypervisor and translate IPA -> PA).

get_page_from_gva may fail because of 1), which is not related to memaccess.

Currently, check_type_get_page emulate only the check for 2). So you may
end up to allow Xen writing in read-only mapping (from the Stage 1 POV).
This was XSA-98.

Regards,

-- 
Julien Grall