diff mbox

[v6,15/36] ARM: introduce vgic_access_guest_memory()

Message ID 20170407173307.9788-16-andre.przywara@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andre Przywara April 7, 2017, 5:32 p.m. UTC
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/8177251

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/vgic.c        | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vgic.h |  3 +++
 2 files changed, 50 insertions(+)

Comments

Julien Grall April 7, 2017, 9:35 p.m. UTC | #1
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@caviumnetworks.com>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  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,
diff mbox

Patch

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;
+
+    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,