[PATCH v3 0/3] xen/pvh: prevent Dom0 from accessing reserved MMIO regions

[PATCH v3 0/3] xen/pvh: prevent Dom0 from accessing reserved MMIO regions

[Roger Pau Monne]

This series aims to prevent PVH Dom0 from accessing MMIO regions of 
devices that are used by Xen.

I know there are concerns with the way we build a PVH Dom0 right now, but 
addressing those requires a major rework of how construct_dom0 works. IMHO I 
don't think this should block this series, but I would understand otherwise.

Thanks for the review, Roger.

[PATCH v3 1/3] xen: allow set_mmio_p2m_entry to specify access type

[Roger Pau Monne]

Preparatory change that allows setting the access type to
set_mmio_p2m_entry.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Eddie Dong <eddie.dong@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Tim Deegan <tim@xen.org>
---
Changes since v2:
 - Latch default access type prior to entering the loop.
---
 xen/arch/x86/domain_build.c |  4 +++-
 xen/arch/x86/hvm/vmx/vmx.c  |  2 +-
 xen/arch/x86/mm/p2m.c       | 15 +++++++++------
 xen/include/asm-x86/p2m.h   |  3 ++-
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 7a912e9..01cfa58 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -319,11 +319,13 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
                                        unsigned long mfn, unsigned long nr_mfns)
 {
     unsigned long i;
+    p2m_access_t a;
     int rc;
 
+    a = p2m_get_hostp2m(d)->default_access;
     for ( i = 0; i < nr_mfns; i++ )
     {
-        if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
+        if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i), a)) )
             panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
                   gfn, mfn, i, rc);
         if ( !(i & 0xfffff) )
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 522892f..ab4b1e5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2173,7 +2173,7 @@ static int vmx_alloc_vlapic_mapping(struct domain *d)
     share_xen_page_with_guest(virt_to_page(apic_va), d, XENSHARE_writable);
     d->arch.hvm_domain.vmx.apic_access_mfn = virt_to_mfn(apic_va);
     set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE),
-        _mfn(virt_to_mfn(apic_va)));
+        _mfn(virt_to_mfn(apic_va)), p2m_get_hostp2m(d)->default_access);
 
     return 0;
 }
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index efa49dd..c1b7545 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -810,7 +810,7 @@ void p2m_change_type_range(struct domain *d,
 
 /* Returns: 0 for success, -errno for failure */
 static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
-                               p2m_type_t gfn_p2mt)
+                               p2m_type_t gfn_p2mt, p2m_access_t access)
 {
     int rc = 0;
     p2m_access_t a;
@@ -837,7 +837,7 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
 
     P2M_DEBUG("set %d %lx %lx\n", gfn_p2mt, gfn, mfn_x(mfn));
     rc = p2m_set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, gfn_p2mt,
-                       p2m->default_access);
+                       access);
     gfn_unlock(p2m, gfn, 0);
     if ( rc )
         gdprintk(XENLOG_ERR,
@@ -850,12 +850,14 @@ static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
 static int set_foreign_p2m_entry(struct domain *d, unsigned long gfn,
                                  mfn_t mfn)
 {
-    return set_typed_p2m_entry(d, gfn, mfn, p2m_map_foreign);
+    return set_typed_p2m_entry(d, gfn, mfn, p2m_map_foreign,
+                               p2m_get_hostp2m(d)->default_access);
 }
 
-int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn)
+int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
+                       p2m_access_t access)
 {
-    return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct);
+    return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct, access);
 }
 
 /* Returns: 0 for success, -errno for failure */
@@ -1858,7 +1860,8 @@ int map_mmio_regions(struct domain *d,
 
     for ( i = 0; !ret && i < nr; i++ )
     {
-        ret = set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i));
+        ret = set_mmio_p2m_entry(d, start_gfn + i, _mfn(mfn + i),
+                                 p2m_get_hostp2m(d)->default_access);
         if ( ret )
         {
             unmap_mmio_regions(d, start_gfn, i, mfn);
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 2cf73ca..e86e26f 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -513,7 +513,8 @@ int p2m_is_logdirty_range(struct p2m_domain *, unsigned long start,
                           unsigned long end);
 
 /* Set mmio addresses in the p2m table (for pass-through) */
-int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
+int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
+                       p2m_access_t access);
 int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn);
 
 /* Add foreign mapping to the guest's p2m table. */
-- 
1.9.3 (Apple Git-50)


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

[PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

[Roger Pau Monne]

Check that MMIO regions added to PVH Dom0 are allowed. Previously a PVH Dom0
would have access to the full MMIO range.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Fix coding style.

Changes since v1:
 - Use the newly introduced p2m_access_t to set the access type.
 - Don't add a next label.
---
 xen/arch/x86/domain_build.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 01cfa58..76722f7 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -320,11 +320,24 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
 {
     unsigned long i;
     p2m_access_t a;
+    mfn_t omfn;
+    p2m_type_t t;
     int rc;
 
-    a = p2m_get_hostp2m(d)->default_access;
     for ( i = 0; i < nr_mfns; i++ )
     {
+        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
+        {
+            omfn = get_gfn_query_unlocked(d, gfn + i, &t);
+            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), PAGE_ORDER_4K);
+            continue;
+        }
+
+        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) )
+            a = p2m_access_r;
+        else
+            a = p2m_access_rw;
+
         if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i), a)) )
             panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
                   gfn, mfn, i, rc);
-- 
1.9.3 (Apple Git-50)


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

[PATCH v3 3/3] xen: prevent access to HPET from Dom0

[Roger Pau Monne]

Prevent Dom0 from accessing HPET MMIO region by adding the HPET mfn to the
list of forbiden memory regions (if ACPI_HPET_PAGE_PROTECT4 flag is set) or
to the list of read-only regions.

Also provide an option that prevents adding the HPET to the read-only memory
regions called ro-hpet, in case there are systems that put other stuff in
the HPET page.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Don't map the HPET page at all if ACPI_HPET_PAGE_PROTECT4 is found.
 - Provide an option (ro-hpet) that prevents adding the HPET page to the
   list of read-only memory regions.

Changes since v1:
 - Instead of completely blocking access to the HPET mfn, set it as
   read-only.
---
 docs/misc/xen-command-line.markdown |  8 ++++++++
 xen/arch/x86/acpi/boot.c            |  1 +
 xen/arch/x86/domain_build.c         | 14 ++++++++++++++
 xen/arch/x86/hpet.c                 |  1 +
 xen/include/asm-x86/hpet.h          |  1 +
 5 files changed, 25 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index a061aa4..e87eef4 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1380,3 +1380,11 @@ Use the x2apic physical apic driver.  The alternative is the x2apic cluster driv
 > Default: `true`
 
 Permit use of the `xsave/xrstor` instructions.
+
+### ro-hpet
+> `= <boolean>`
+
+> Default: `true`
+
+Map the HPET page as read only in Dom0. If disabled the page will be mapped
+with read and write permissions.
diff --git a/xen/arch/x86/acpi/boot.c b/xen/arch/x86/acpi/boot.c
index 903830b..9a8904b 100644
--- a/xen/arch/x86/acpi/boot.c
+++ b/xen/arch/x86/acpi/boot.c
@@ -309,6 +309,7 @@ static int __init acpi_parse_hpet(struct acpi_table_header *table)
 
 	hpet_address = hpet_tbl->address.address;
 	hpet_blockid = hpet_tbl->sequence;
+	hpet_flags = hpet_tbl->flags;
 	printk(KERN_INFO PREFIX "HPET id: %#x base: %#lx\n",
 	       hpet_tbl->id, hpet_address);
 
diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 76722f7..85c47cc 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -36,6 +36,7 @@
 #include <asm/bzimage.h> /* for bzimage_parse */
 #include <asm/io_apic.h>
 #include <asm/hap.h>
+#include <asm/hpet.h> /* for hpet_address */
 
 #include <public/version.h>
 
@@ -134,6 +135,9 @@ boolean_param("dom0_shadow", opt_dom0_shadow);
 static char __initdata opt_dom0_ioports_disable[200] = "";
 string_param("dom0_ioports_disable", opt_dom0_ioports_disable);
 
+static bool_t __initdata ro_hpet = 1;
+boolean_param("ro-hpet", ro_hpet);
+
 /* Allow ring-3 access in long mode as guest cannot use ring 1 ... */
 #define BASE_PROT (_PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_USER)
 #define L1_PROT (BASE_PROT|_PAGE_GUEST_KERNEL)
@@ -1495,6 +1499,16 @@ int __init construct_dom0(
             rc |= iomem_deny_access(d, sfn, efn);
     }
 
+    /* Prevent access to HPET */
+    if ( hpet_address != 0 )
+    {
+        mfn = paddr_to_pfn(hpet_address);
+        if ( hpet_flags & ACPI_HPET_PAGE_PROTECT4 )
+            rc |= iomem_deny_access(d, mfn, mfn);
+        else if ( ro_hpet )
+            rc |= rangeset_add_singleton(mmio_ro_ranges, mfn);
+    }
+
     BUG_ON(rc != 0);
 
     if ( elf_check_broken(&elf) )
diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 0b13f52..7aa740f 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -52,6 +52,7 @@ DEFINE_PER_CPU(struct hpet_event_channel *, cpu_bc_channel);
 
 unsigned long __initdata hpet_address;
 u8 __initdata hpet_blockid;
+u8 __initdata hpet_flags;
 
 /*
  * force_hpet_broadcast: by default legacy hpet broadcast will be stopped
diff --git a/xen/include/asm-x86/hpet.h b/xen/include/asm-x86/hpet.h
index 875f1de..10c4a56 100644
--- a/xen/include/asm-x86/hpet.h
+++ b/xen/include/asm-x86/hpet.h
@@ -52,6 +52,7 @@
 
 extern unsigned long hpet_address;
 extern u8 hpet_blockid;
+extern u8 hpet_flags;
 
 /*
  * Detect and initialise HPET hardware: return counter update frequency.
-- 
1.9.3 (Apple Git-50)


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

Re: [PATCH v3 1/3] xen: allow set_mmio_p2m_entry to specify access type

[Tim Deegan]

At 16:19 +0100 on 22 Jan (1421939961), Roger Pau Monne wrote:
> Preparatory change that allows setting the access type to
> set_mmio_p2m_entry.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Eddie Dong <eddie.dong@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>

Acked-by: Tim Deegan <tim@xen.org>

Re: [PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

[Jan Beulich]

>>> On 22.01.15 at 16:19, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -320,11 +320,24 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
>  {
>      unsigned long i;
>      p2m_access_t a;
> +    mfn_t omfn;
> +    p2m_type_t t;
>      int rc;
>  
> -    a = p2m_get_hostp2m(d)->default_access;

Iirc this is rwx.

>      for ( i = 0; i < nr_mfns; i++ )
>      {
> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
> +        {
> +            omfn = get_gfn_query_unlocked(d, gfn + i, &t);
> +            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), PAGE_ORDER_4K);
> +            continue;
> +        }
> +
> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) )
> +            a = p2m_access_r;
> +        else
> +            a = p2m_access_rw;

Shouldn't these two therefore be rx and rwx respectively? Or even
better ->default_access in the else case (albeit that doesn't really
matter here since nothing can have changed that field from its
default value)? I'm particularly thinking of ROMs that may be sitting
in these areas.

Jan

Re: [PATCH v3 3/3] xen: prevent access to HPET from Dom0

[Jan Beulich]

>>> On 22.01.15 at 16:19, <roger.pau@citrix.com> wrote:
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -36,6 +36,7 @@
>  #include <asm/bzimage.h> /* for bzimage_parse */
>  #include <asm/io_apic.h>
>  #include <asm/hap.h>
> +#include <asm/hpet.h> /* for hpet_address */

Please drop the comment - with hpet_flags it's now stale

> @@ -1495,6 +1499,16 @@ int __init construct_dom0(
>              rc |= iomem_deny_access(d, sfn, efn);
>      }
>  
> +    /* Prevent access to HPET */
> +    if ( hpet_address != 0 )
> +    {
> +        mfn = paddr_to_pfn(hpet_address);
> +        if ( hpet_flags & ACPI_HPET_PAGE_PROTECT4 )

The constant isn't a binary mask, you need to also use
ACPI_HPET_PAGE_PROTECT_MASK. I further can't see why
you wouldn't want to also handle ACPI_HPET_PAGE_PROTECT64.

Jan

Re: [PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

[Konrad Rzeszutek Wilk]

On Thu, Jan 22, 2015 at 04:19:22PM +0100, Roger Pau Monne wrote:
> Check that MMIO regions added to PVH Dom0 are allowed. Previously a PVH Dom0
> would have access to the full MMIO range.

How do we do this for normal PV dom0? Do we enforce the same
restriction? If not, should we ?
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Changes since v2:
>  - Fix coding style.
> 
> Changes since v1:
>  - Use the newly introduced p2m_access_t to set the access type.
>  - Don't add a next label.
> ---
>  xen/arch/x86/domain_build.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index 01cfa58..76722f7 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -320,11 +320,24 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
>  {
>      unsigned long i;
>      p2m_access_t a;
> +    mfn_t omfn;
> +    p2m_type_t t;
>      int rc;
>  
> -    a = p2m_get_hostp2m(d)->default_access;
>      for ( i = 0; i < nr_mfns; i++ )
>      {
> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
> +        {
> +            omfn = get_gfn_query_unlocked(d, gfn + i, &t);
> +            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), PAGE_ORDER_4K);
> +            continue;
> +        }
> +
> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) )
> +            a = p2m_access_r;
> +        else
> +            a = p2m_access_rw;
> +
>          if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i), a)) )
>              panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
>                    gfn, mfn, i, rc);
> -- 
> 1.9.3 (Apple Git-50)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

[Jan Beulich]

>>> On 22.01.15 at 21:28, <konrad.wilk@oracle.com> wrote:
> On Thu, Jan 22, 2015 at 04:19:22PM +0100, Roger Pau Monne wrote:
>> Check that MMIO regions added to PVH Dom0 are allowed. Previously a PVH Dom0
>> would have access to the full MMIO range.
> 
> How do we do this for normal PV dom0? Do we enforce the same
> restriction?

We do, at the MMU level at least. Thing is (see an earlier
reply, maybe on Elena's thread) that we may still be too lax.

Jan

Re: [PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

[Roger Pau Monné]

El 22/01/15 a les 16.43, Jan Beulich ha escrit:
>>>> On 22.01.15 at 16:19, <roger.pau@citrix.com> wrote:
>> --- a/xen/arch/x86/domain_build.c
>> +++ b/xen/arch/x86/domain_build.c
>> @@ -320,11 +320,24 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
>>  {
>>      unsigned long i;
>>      p2m_access_t a;
>> +    mfn_t omfn;
>> +    p2m_type_t t;
>>      int rc;
>>  
>> -    a = p2m_get_hostp2m(d)->default_access;
> 
> Iirc this is rwx.
> 
>>      for ( i = 0; i < nr_mfns; i++ )
>>      {
>> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
>> +        {
>> +            omfn = get_gfn_query_unlocked(d, gfn + i, &t);
>> +            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), PAGE_ORDER_4K);
>> +            continue;
>> +        }
>> +
>> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) )
>> +            a = p2m_access_r;
>> +        else
>> +            a = p2m_access_rw;
> 
> Shouldn't these two therefore be rx and rwx respectively? Or even
> better ->default_access in the else case (albeit that doesn't really
> matter here since nothing can have changed that field from its
> default value)? I'm particularly thinking of ROMs that may be sitting
> in these areas.

Yes, it should be rx and rwx. I would prefer to use the explicit types.
IMHO it's easier to understand to which access type it's set (without
having to dig to what value p2m_get_hostp2m(d)->default_access is set).

Roger.

Re: [PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

[Jan Beulich]

>>> On 23.01.15 at 12:29, <roger.pau@citrix.com> wrote:
> El 22/01/15 a les 16.43, Jan Beulich ha escrit:
>>>>> On 22.01.15 at 16:19, <roger.pau@citrix.com> wrote:
>>> --- a/xen/arch/x86/domain_build.c
>>> +++ b/xen/arch/x86/domain_build.c
>>> @@ -320,11 +320,24 @@ static __init void pvh_add_mem_mapping(struct domain 
> *d, unsigned long gfn,
>>>  {
>>>      unsigned long i;
>>>      p2m_access_t a;
>>> +    mfn_t omfn;
>>> +    p2m_type_t t;
>>>      int rc;
>>>  
>>> -    a = p2m_get_hostp2m(d)->default_access;
>> 
>> Iirc this is rwx.
>> 
>>>      for ( i = 0; i < nr_mfns; i++ )
>>>      {
>>> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
>>> +        {
>>> +            omfn = get_gfn_query_unlocked(d, gfn + i, &t);
>>> +            guest_physmap_remove_page(d, gfn + i, mfn_x(omfn), 
> PAGE_ORDER_4K);
>>> +            continue;
>>> +        }
>>> +
>>> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) )
>>> +            a = p2m_access_r;
>>> +        else
>>> +            a = p2m_access_rw;
>> 
>> Shouldn't these two therefore be rx and rwx respectively? Or even
>> better ->default_access in the else case (albeit that doesn't really
>> matter here since nothing can have changed that field from its
>> default value)? I'm particularly thinking of ROMs that may be sitting
>> in these areas.
> 
> Yes, it should be rx and rwx. I would prefer to use the explicit types.
> IMHO it's easier to understand to which access type it's set (without
> having to dig to what value p2m_get_hostp2m(d)->default_access is set).

That's fine then.

Jan

Re: [PATCH v3 3/3] xen: prevent access to HPET from Dom0

[Roger Pau Monné]

El 22/01/15 a les 16.47, Jan Beulich ha escrit:
>>>> On 22.01.15 at 16:19, <roger.pau@citrix.com> wrote:
>> --- a/xen/arch/x86/domain_build.c
>> +++ b/xen/arch/x86/domain_build.c
>> @@ -36,6 +36,7 @@
>>  #include <asm/bzimage.h> /* for bzimage_parse */
>>  #include <asm/io_apic.h>
>>  #include <asm/hap.h>
>> +#include <asm/hpet.h> /* for hpet_address */
> 
> Please drop the comment - with hpet_flags it's now stale
> 
>> @@ -1495,6 +1499,16 @@ int __init construct_dom0(
>>              rc |= iomem_deny_access(d, sfn, efn);
>>      }
>>  
>> +    /* Prevent access to HPET */
>> +    if ( hpet_address != 0 )
>> +    {
>> +        mfn = paddr_to_pfn(hpet_address);
>> +        if ( hpet_flags & ACPI_HPET_PAGE_PROTECT4 )
> 
> The constant isn't a binary mask, you need to also use
> ACPI_HPET_PAGE_PROTECT_MASK. I further can't see why
> you wouldn't want to also handle ACPI_HPET_PAGE_PROTECT64.

Right, for PAGE_PROTECT64 we can also block access to the adjacent 15 pages.

Roger.

Re: [PATCH v3 2/3] xen/pvh: check permissions when adding MMIO regions

[Konrad Rzeszutek Wilk]

On Fri, Jan 23, 2015 at 11:21:11AM +0000, Jan Beulich wrote:
> >>> On 22.01.15 at 21:28, <konrad.wilk@oracle.com> wrote:
> > On Thu, Jan 22, 2015 at 04:19:22PM +0100, Roger Pau Monne wrote:
> >> Check that MMIO regions added to PVH Dom0 are allowed. Previously a PVH Dom0
> >> would have access to the full MMIO range.
> > 
> > How do we do this for normal PV dom0? Do we enforce the same
> > restriction?
> 
> We do, at the MMU level at least. Thing is (see an earlier
> reply, maybe on Elena's thread) that we may still be too lax.

Looking at vtd_set_hwdom_mapping looks to be having quite simplified
mechanism for PV. This is of course for those weird devices that
do DMA operations behind the OS'es back.

If we go that route we would probablly need logic in there to change
too - otherwise we have .. Ah, this is where Tim's idea of having
a guest just having an flag of 'I_need_IOMMU' would be so nice and
having only one code-path that could be used for both PV and PVH.


> 
> Jan
>