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

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-03-12 at 14:52 +0000, Julien Grall wrote:
> 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.

XSA-98 was purely about stage-2 permissions (e.g. read-only grants). The
fact that the resulting patch also checks stage-1 permissions is not a
security property AFAICT.

Ian.