From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages Date: Thu, 12 Mar 2015 15:27:35 +0000 Message-ID: <1426174055.32572.10.camel@citrix.com> References: <1425677073-13729-1-git-send-email-tklengyel@sec.in.tum.de> <1425677073-13729-4-git-send-email-tklengyel@sec.in.tum.de> <55019996.9050208@linaro.org> <5501A831.6010009@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5501A831.6010009@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: wei.liu2@citrix.com, Stefano Stabellini , Tim Deegan , Ian Jackson , xen-devel@lists.xen.org, stefano.stabellini@citrix.com, Jan Beulich , Keir Fraser , Tamas K Lengyel List-Id: xen-devel@lists.xenproject.org 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 > > 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.