From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH V13 3/7] xen/arm: Allow hypervisor access to mem_access protected pages Date: Thu, 12 Mar 2015 13:24:18 +0000 Message-ID: <55019382.4040108@linaro.org> References: <1425677073-13729-1-git-send-email-tklengyel@sec.in.tum.de> <1425677073-13729-4-git-send-email-tklengyel@sec.in.tum.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1425677073-13729-4-git-send-email-tklengyel@sec.in.tum.de> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel , xen-devel@lists.xen.org Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, tim@xen.org, stefano.stabellini@citrix.com, jbeulich@suse.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org Hi Tamas, On 06/03/15 21:24, Tamas K Lengyel wrote: > The guestcopy helpers use the MMU to verify that the given guest has read/write > access to a given page during hypercalls. As we may have custom mem_access > permissions set on these pages, we do a software-based type checking in case > the MMU based approach failed, but only if mem_access_enabled is set. > > These memory accesses are not forwarded to the mem_event listener. Accesses > performed by the hypervisor are currently not part of the mem_access scheme. > This is consistent behaviour with the x86 side as well. > > Signed-off-by: Tamas K Lengyel > --- > v12: - Check for mfn_valid as well. > --- > xen/arch/arm/guestcopy.c | 124 +++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 121 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c > index 7dbaeca..d42a469 100644 > --- a/xen/arch/arm/guestcopy.c > +++ b/xen/arch/arm/guestcopy.c > @@ -6,6 +6,115 @@ > > #include > #include > +#include > + > +/* > + * 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; AFAICT, all the return value stored in rc are int. > + paddr_t ipa; > + unsigned long maddr; > + unsigned long mfn; > + xenmem_access_t xma; > + p2m_type_t t; > + > + rc = gva_to_ipa(gva, &ipa); > + if ( rc < 0 ) > + return rc; > + > + /* > + * We do this first as this is faster in the default case when no > + * permission is set on the page. > + */ > + rc = p2m_get_mem_access(current->domain, paddr_to_pfn(ipa), &xma); > + if ( rc < 0 ) Maybe a likely would be good here? > + return rc; > + > + /* Let's check if mem_access limited the access. */ > + switch ( xma ) > + { > + default: > + case XENMEM_access_rwx: > + case XENMEM_access_rw: > + return -EFAULT; > + case XENMEM_access_n2rwx: > + case XENMEM_access_n: > + case XENMEM_access_x: > + break; > + case XENMEM_access_wx: > + case XENMEM_access_w: > + if ( flag == GV2M_READ ) > + break; > + else return -EFAULT; > + case XENMEM_access_rx2rw: > + case XENMEM_access_rx: > + case XENMEM_access_r: > + if ( flag == GV2M_WRITE ) > + break; > + else return -EFAULT; > + } > + > + /* > + * We had a mem_access permission limiting the access, but the page type > + * could also be limiting, so we need to check that as well. > + */ > + maddr = p2m_lookup(current->domain, ipa, &t); > + if ( maddr == INVALID_PADDR ) > + return -EFAULT; > + > + mfn = maddr >> PAGE_SHIFT; > + if ( !mfn_valid(mfn) ) > + return -EFAULT; > + > + /* > + * All page types are readable so we only have to check the > + * type if writing. > + */ That's may change in the future. A white-list may be better in order to avoid giving wrong access when a new p2m type is added. > + if ( flag == GV2M_WRITE ) > + { > + switch ( t ) > + { > + case p2m_ram_rw: > + case p2m_iommu_map_rw: > + case p2m_map_foreign: > + case p2m_grant_map_rw: > + case p2m_mmio_direct: We disallow guest copy from the above 4 types via get_page_from_gva. So I'm not sure if it's worth to check them here. Regards, -- Julien Grall