On Thu, Mar 12, 2015 at 1:08 PM, Ian Campbell <ian.campb...@citrix.com>
wrote:
> On Fri, 2015-03-06 at 22:24 +0100, 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 <tkleng...@sec.in.tum.de>
> > ---
> > 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 <asm/mm.h>
> > #include <asm/guest_access.h>
> > +#include <asm/p2m.h>
> > +
> > +/*
> > + * 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);
> > + 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 )
> > + return rc;
> > +
> > + /* Let's check if mem_access limited the access. */
> > + switch ( xma )
> > + {
> > + default:
> > + case XENMEM_access_rwx:
> > + case XENMEM_access_rw:
>
> If I've understood correctly then this is deliberately backwards
> looking.
>
> i.e. if we have gotten here then we have failed an earlier
> get_page_from_gva check, so if xma is XENMEM_access_rwx that means that
> the result of that first get_page_from_gva is valid because memaccess
> hasn't been messing with the permissions.
>
> Since this interface only does reads or writes and not executableness
> rwx and rw are effectively the same.
>
> Is my understanding correct?
>
Yes, correct. If mem_access had no r/w restriction on this page, the result
of get_page_from_gva is valid.
>
> > + return -EFAULT;
> > + case XENMEM_access_n2rwx:
> > + case XENMEM_access_n:
> > + case XENMEM_access_x:
> > + break;
>
> If xenaccess contains no rw perms at all then we need to check what the
> original p2m type was in order to decide what to do.
>
Correct.
>
> > + case XENMEM_access_wx:
> > + case XENMEM_access_w:
> > + if ( flag == GV2M_READ )
> > + break;
> > + else return -EFAULT;
>
> If thing was a read then it might be because of xenaccess, so we must
> check, but if it was a write then the origianl get_page_from_gva fault
> was correct.
>
Correct, mem_access here had a write restriction but get_page_from_gva
faulted with read, so that fault was correct.
>
>
> > + case XENMEM_access_rx2rw:
> > + case XENMEM_access_rx:
> > + case XENMEM_access_r:
> > + if ( flag == GV2M_WRITE )
> > + break;
> > + else return -EFAULT;
>
> and vice versa.
>
> (sorry to be so tedious, I wanted to work through them all to ensure I'd
> understood correctly and that they were right).
>
Yes, this is a bit complex but your understanding is correct. I will add a
comment describing it in human readable way as Julien also had the same
question couple months ago.
>
> > @@ -68,7 +180,10 @@ unsigned long raw_clear_guest(void *to, unsigned len)
> >
> > page = get_page_from_gva(current->domain, (vaddr_t) to,
> GV2M_WRITE);
> > if ( page == NULL )
> > - return len;
> > + {
> > + if ( check_mem_access((vaddr_t) to, GV2M_WRITE, &page) < 0 )
> > + return len;
> > + }
>
> I think this would be better done by a making check_mem_access include
> the initial call to get_page_from_gva and calling that helper here
> instead instead of repeating this code.
>
> I'd even consider putting the memaccess check as a call at the tail end
> of get_page_from_gva, I don't think there are any callers who don't want
> this behaviour.
>
Sure, that sounds reasonable.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
