Re: [Xen-devel] [PATCH v6 15/36] ARM: introduce vgic_access_guest_memory()

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 04/07/2017 06:32 PM, Andre Przywara wrote:
> This function allows to copy a chunk of data from and to guest physical
> memory. It looks up the associated page from the guest's p2m tree
> and maps this page temporarily for the time of the access.
> This function was originally written by Vijaya as part of an earlier series:
> https://patchwork.kernel.org/patch/8177251i
This likely means the From: should be "Vijaya...".

> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/vgic.c        | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vgic.h |  3 +++
>  2 files changed, 50 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index e6c97b2..5c68fe7 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -20,6 +20,7 @@
>  #include <xen/bitops.h>
>  #include <xen/lib.h>
>  #include <xen/init.h>
> +#include <xen/domain_page.h>
>  #include <xen/softirq.h>
>  #include <xen/irq.h>
>  #include <xen/sched.h>
> @@ -598,6 +599,52 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
>  }
>
>  /*
> + * Temporarily map one physical guest page and copy data to or from it.
> + * The data to be copied cannot cross a page boundary.
> + */
> +int vgic_access_guest_memory(struct domain *d, paddr_t gpa, void *buf,
> +                             uint32_t size, bool_t is_write)
> +{
> +    struct page_info *page;
> +    uint64_t offset;
> +    p2m_type_t p2mt;
> +    void *p;
> +
> +    page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC);
> +    if ( !page )
> +    {
> +        printk(XENLOG_G_ERR "d%d: vITS: Failed to get table entry\n",
> +               d->domain_id);
> +        return -EINVAL;
> +    }
> +
> +    if ( !p2m_is_ram(p2mt) )
> +    {
> +        put_page(page);
> +        printk(XENLOG_G_ERR "d%d: vITS: memory used by the ITS should be RAM.",
> +               d->domain_id);
> +        return -EINVAL;
> +    }
> +
> +    p = __map_domain_page(page);
> +    /* Offset within the mapped page */
> +    offset = gpa & ~PAGE_MASK;
> +    /* Do not cross a page boundary. */
> +    if ( size > (PAGE_SIZE - offset) )
> +        size = PAGE_SIZE - offset;
Hmmmm.... it is not what I meant by a check. If a user uses this 
function he will expect *all* the buffer filled and not only a part if a 
page boundary is crossed. This would be a potential security issue if 
the buffer is then written back to the guest memory (I know this is not 
the case today, but still...).
I think it would be better if we return -EINVAL if the in this case.

> +
> +    if ( is_write )
> +        memcpy(p + offset, buf, size);
> +    else
> +        memcpy(buf, p + offset, size);
> +
> +    unmap_domain_page(p);
> +    put_page(page);
> +
> +    return 0;
> +}
> +
> +/*
>   * Local variables:
>   * mode: C
>   * c-file-style: "BSD"
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 52856ca..967c2be 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -319,6 +319,9 @@ extern void register_vgic_ops(struct domain *d, const 
> struct vgic_ops *ops);
>  int vgic_v2_init(struct domain *d, int *mmio_count);
>  int vgic_v3_init(struct domain *d, int *mmio_count);
>
> +int vgic_access_guest_memory(struct domain *d, paddr_t gpa, void *buf,
> +                             uint32_t size, bool_t is_write);
> +
>  extern int domain_vgic_register(struct domain *d, int *mmio_count);
>  extern int vcpu_vgic_free(struct vcpu *v);
>  extern bool vgic_to_sgi(struct vcpu *v, register_t sgir,
Cheers,

--
Julien Grall

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