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

[Tamas K Lengyel <tklengyel@xxxxxxxxxxxxx>]

On Thu, Mar 12, 2015 at 2:24 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:

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 <tklengyel@xxxxxxxxxxxxx>
> ---
> 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;

AFAICT, all the return value stored in rc are int.

Ack. I actually relocated this function into p2m.c in the next iteration of the series and change the return type to struct page_info* page.

Â

> +Â Â 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?

Sure.

Â

> +Â Â Â Â 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.

Hm, sure.

Â

> +Â Â 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.

You mean get_page_from_gva only works for p2m_ram_rw type? Is this the case for GV2M_READ as well?

Â

Regards,

--
Julien Grall

Thanks,

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel