[PATCH v1 for-4.6 1/2] xen: fixes for PVH Dom0 MMIO regions

[PATCH v1 for-4.6 1/2] xen: fixes for PVH Dom0 MMIO regions

[Roger Pau Monne]

Hello,

This series contains a bug-fix for PVH Dom0, that prevents Xen from adding 
MMIO regions that should not be accesible to Dom0. The second patch also 
prevents Dom0 from accessing the HPET, which AFAICT is used by Xen.

I'm not sure if there's a reason why the HPET MMIO region wasn't added to 
iomem_deny_access, but I don't think Dom0 should access it.

[PATCH v1 for-4.6 1/2] 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>
---
 xen/arch/x86/domain_build.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 7a6afea..aa3bf0f 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -312,16 +312,47 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
                                        unsigned long mfn, unsigned long nr_mfns)
 {
     unsigned long i;
+    xenmem_access_t def_access;
+    bool_t r_only = false;
     int rc;
 
     for ( i = 0; i < nr_mfns; i++ )
     {
+        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
+            goto next;
+
+        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) && !r_only )
+        {
+            rc = p2m_get_mem_access(d, ~0ul, &def_access);
+            BUG_ON(rc);
+            /* Set default access to read-only */
+            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, XENMEM_access_r);
+            BUG_ON(rc);
+            r_only = true;
+        }
+        else if ( r_only )
+        {
+            /* Set the default back */
+            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);
+            BUG_ON(rc);
+            r_only = false;
+        }
+
         if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
             panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
                   gfn, mfn, i, rc);
+
+ next:
         if ( !(i & 0xfffff) )
                 process_pending_softirqs();
     }
+
+    if ( r_only )
+    {
+        /* Set the default back */
+        rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);
+        BUG_ON(rc);
+    }
 }
 
 /*
-- 
1.9.3 (Apple Git-50)


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

[PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Roger Pau Monne]

Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
denied memory regions.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/domain_build.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index aa3bf0f..788f7db 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -36,6 +36,9 @@
 #include <asm/bzimage.h> /* for bzimage_parse */
 #include <asm/io_apic.h>
 #include <asm/hap.h>
+#ifdef CONFIG_HPET_TIMER
+#include <asm/hpet.h> /* for hpet_address */
+#endif
 
 #include <public/version.h>
 
@@ -1516,6 +1519,15 @@ int __init construct_dom0(
             rc |= iomem_deny_access(d, sfn, efn);
     }
 
+#ifdef CONFIG_HPET_TIMER
+    /* Prevent access to HPET */
+    if ( hpet_address != 0 )
+    {
+        mfn = paddr_to_pfn(hpet_address);
+        rc |= iomem_deny_access(d, mfn, mfn);
+    }
+#endif
+
     BUG_ON(rc != 0);
 
     if ( elf_check_broken(&elf) )
-- 
1.9.3 (Apple Git-50)


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

Re: [PATCH v1 for-4.6 1/2] xen: fixes for PVH Dom0 MMIO regions

[Andrew Cooper]

On 18/12/14 18:27, Roger Pau Monne wrote:
> Hello,
>
> This series contains a bug-fix for PVH Dom0, that prevents Xen from adding 
> MMIO regions that should not be accesible to Dom0. The second patch also 
> prevents Dom0 from accessing the HPET, which AFAICT is used by Xen.
>
> I'm not sure if there's a reason why the HPET MMIO region wasn't added to 
> iomem_deny_access, but I don't think Dom0 should access it.

The HPET region is awkward.  It is only 1024 bytes wide.

Dom0 may legitimately need access to other MMIO which lives in the
remainder of page.

Having said that, the HPET ACPI table does have a flag indicating that
the HPET page has nothing else in the remainder of the page.  We
probably should deny dom0 access in the case that the BIOS has told us
it is safe to do so.

~Andrew

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Andrew Cooper]

On 18/12/14 18:27, Roger Pau Monne wrote:
> Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
> denied memory regions.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Apologies that this reply is split between patch 0 and 2 - I replied to
your cover letter before reading this patch.

Denying access is only valid if acpi_table_hpet.flags & 
ACPI_HPET_PAGE_PROTECT4 is true.

~Andrew

> ---
>  xen/arch/x86/domain_build.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index aa3bf0f..788f7db 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -36,6 +36,9 @@
>  #include <asm/bzimage.h> /* for bzimage_parse */
>  #include <asm/io_apic.h>
>  #include <asm/hap.h>
> +#ifdef CONFIG_HPET_TIMER
> +#include <asm/hpet.h> /* for hpet_address */
> +#endif
>  
>  #include <public/version.h>
>  
> @@ -1516,6 +1519,15 @@ int __init construct_dom0(
>              rc |= iomem_deny_access(d, sfn, efn);
>      }
>  
> +#ifdef CONFIG_HPET_TIMER
> +    /* Prevent access to HPET */
> +    if ( hpet_address != 0 )
> +    {
> +        mfn = paddr_to_pfn(hpet_address);
> +        rc |= iomem_deny_access(d, mfn, mfn);
> +    }
> +#endif
> +
>      BUG_ON(rc != 0);
>  
>      if ( elf_check_broken(&elf) )


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

Re: [PATCH v1 for-4.6 1/2] xen/pvh: check permissions when adding MMIO regions

[Andrew Cooper]

On 18/12/14 18:27, 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.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/domain_build.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index 7a6afea..aa3bf0f 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -312,16 +312,47 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
>                                         unsigned long mfn, unsigned long nr_mfns)
>  {
>      unsigned long i;
> +    xenmem_access_t def_access;
> +    bool_t r_only = false;
>      int rc;
>  
>      for ( i = 0; i < nr_mfns; i++ )
>      {
> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
> +            goto next;
> +
> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) && !r_only )
> +        {
> +            rc = p2m_get_mem_access(d, ~0ul, &def_access);
> +            BUG_ON(rc);
> +            /* Set default access to read-only */
> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, XENMEM_access_r);
> +            BUG_ON(rc);
> +            r_only = true;
> +        }
> +        else if ( r_only )
> +        {
> +            /* Set the default back */
> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);
> +            BUG_ON(rc);
> +            r_only = false;
> +        }

Am I missing something obvious, or would all this r_only juggling be far
more easy if set_mmio_p2m_entry() had a ro/rw boolean parameter?

As these entries are done one at a time, it would seem to be far less
error prone to explicitly choose a read-only or read-write mmio mapping,
rather than playing with the entire default.

> +
>          if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
>              panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
>                    gfn, mfn, i, rc);
> +
> + next:
>          if ( !(i & 0xfffff) )
>                  process_pending_softirqs();

You could remove the next label by moving this clause to the top and
adding an i != 0 check.

>      }
> +
> +    if ( r_only )
> +    {
> +        /* Set the default back */
> +        rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);

This will clobber the p2m default access type based on whether the final
mfn is read-only or read-write.

~Andrew

> +        BUG_ON(rc);
> +    }
>  }
>  
>  /*



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

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Roger Pau Monné]

Hello,

El 18/12/14 a les 19.51, Andrew Cooper ha escrit:
> On 18/12/14 18:27, Roger Pau Monne wrote:
>> Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
>> denied memory regions.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Apologies that this reply is split between patch 0 and 2 - I replied to
> your cover letter before reading this patch.
> 
> Denying access is only valid if acpi_table_hpet.flags & 
> ACPI_HPET_PAGE_PROTECT4 is true.

Thanks, if ACPI_HPET_PAGE_PROTECT4 is set then we can prevent access to
the full page and if ACPI_HPET_PAGE_PROTECT64 is set we can prevent
access to this page and the adjacent 60KB (15 pages). Will send an
updated version.

Roger.


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

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Jan Beulich]

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

When you update the patch according to Andrew's comments, please
also drop these stray #ifdef-s - if anything we should delete eventual
other references to CONFIG_HPET_TIMER.

Jan

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Jan Beulich]

>>> On 18.12.14 at 19:51, <andrew.cooper3@citrix.com> wrote:
> On 18/12/14 18:27, Roger Pau Monne wrote:
>> Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
>> denied memory regions.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Apologies that this reply is split between patch 0 and 2 - I replied to
> your cover letter before reading this patch.
> 
> Denying access is only valid if acpi_table_hpet.flags & 
> ACPI_HPET_PAGE_PROTECT4 is true.

Somehow the existence of this indication slipped my attention so far;
I had always wanted to hide the HPET page from Dom0 if possible.

Jan

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

Re: [PATCH v1 for-4.6 1/2] xen/pvh: check permissions when adding MMIO regions

[Jan Beulich]

>>> On 18.12.14 at 20:05, <andrew.cooper3@citrix.com> wrote:
> On 18/12/14 18:27, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/domain_build.c
>> +++ b/xen/arch/x86/domain_build.c
>> @@ -312,16 +312,47 @@ static __init void pvh_add_mem_mapping(struct domain *d, unsigned long gfn,
>>                                         unsigned long mfn, unsigned long nr_mfns)
>>  {
>>      unsigned long i;
>> +    xenmem_access_t def_access;
>> +    bool_t r_only = false;
>>      int rc;
>>  
>>      for ( i = 0; i < nr_mfns; i++ )
>>      {
>> +        if ( !iomem_access_permitted(d, mfn + i, mfn + i) )
>> +            goto next;
>> +
>> +        if ( rangeset_contains_singleton(mmio_ro_ranges, mfn + i) && !r_only )
>> +        {
>> +            rc = p2m_get_mem_access(d, ~0ul, &def_access);
>> +            BUG_ON(rc);
>> +            /* Set default access to read-only */
>> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, XENMEM_access_r);
>> +            BUG_ON(rc);
>> +            r_only = true;
>> +        }
>> +        else if ( r_only )
>> +        {
>> +            /* Set the default back */
>> +            rc = p2m_set_mem_access(d, ~0ul, 0, 0, 0, def_access);
>> +            BUG_ON(rc);
>> +            r_only = false;
>> +        }
> 
> Am I missing something obvious, or would all this r_only juggling be far
> more easy if set_mmio_p2m_entry() had a ro/rw boolean parameter?
> 
> As these entries are done one at a time, it would seem to be far less
> error prone to explicitly choose a read-only or read-write mmio mapping,
> rather than playing with the entire default.

+1

>> +
>>          if ( (rc = set_mmio_p2m_entry(d, gfn + i, _mfn(mfn + i))) )
>>              panic("pvh_add_mem_mapping: gfn:%lx mfn:%lx i:%ld rc:%d\n",
>>                    gfn, mfn, i, rc);
>> +
>> + next:
>>          if ( !(i & 0xfffff) )
>>                  process_pending_softirqs();
> 
> You could remove the next label by moving this clause to the top and
> adding an i != 0 check.

Or really just make the respective goto a continue - we know we
don't hide overly large regions from Dom0.

Jan

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Jan Beulich]

>>> On 18.12.14 at 19:51, <andrew.cooper3@citrix.com> wrote:
> On 18/12/14 18:27, Roger Pau Monne wrote:
>> Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
>> denied memory regions.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Apologies that this reply is split between patch 0 and 2 - I replied to
> your cover letter before reading this patch.
> 
> Denying access is only valid if acpi_table_hpet.flags & 
> ACPI_HPET_PAGE_PROTECT4 is true.

Having just checked (as an example) the most modern Intel box I
have direct access to, I wonder how many systems actually supply
other than 0 here. Perhaps we ought to at once add a command
line option to trigger the denial?

Jan

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

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Andrew Cooper]

On 19/12/14 08:04, Roger Pau Monné wrote:
> Hello,
>
> El 18/12/14 a les 19.51, Andrew Cooper ha escrit:
>> On 18/12/14 18:27, Roger Pau Monne wrote:
>>> Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
>>> denied memory regions.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Apologies that this reply is split between patch 0 and 2 - I replied to
>> your cover letter before reading this patch.
>>
>> Denying access is only valid if acpi_table_hpet.flags & 
>> ACPI_HPET_PAGE_PROTECT4 is true.
> Thanks, if ACPI_HPET_PAGE_PROTECT4 is set then we can prevent access to
> the full page and if ACPI_HPET_PAGE_PROTECT64 is set we can prevent
> access to this page and the adjacent 60KB (15 pages). Will send an
> updated version.
>
> Roger.
>

ACPI_HPET_PAGE_PROTECT64 stems from ia64 land, where pages can be 64k. 
I believe it can safely be ignored for x86, or perhaps used to imply
ACPI_HPET_PAGE_PROTECT4

~Andrew


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

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Andrew Cooper]

On 19/12/14 09:11, Jan Beulich wrote:
>>>> On 18.12.14 at 19:51, <andrew.cooper3@citrix.com> wrote:
>> On 18/12/14 18:27, Roger Pau Monne wrote:
>>> Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
>>> denied memory regions.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Apologies that this reply is split between patch 0 and 2 - I replied to
>> your cover letter before reading this patch.
>>
>> Denying access is only valid if acpi_table_hpet.flags & 
>> ACPI_HPET_PAGE_PROTECT4 is true.
> Having just checked (as an example) the most modern Intel box I
> have direct access to, I wonder how many systems actually supply
> other than 0 here. Perhaps we ought to at once add a command
> line option to trigger the denial?

I also can't find a server which sets this flag.  I wonder how many
systems actually have other things sitting in the remainder of the page.

~Andrew


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

Re: [PATCH v1 for-4.6 2/2] xen: prevent access to HPET from Dom0

[Jan Beulich]

>>> On 19.12.14 at 12:32, <andrew.cooper3@citrix.com> wrote:
> On 19/12/14 09:11, Jan Beulich wrote:
>>>>> On 18.12.14 at 19:51, <andrew.cooper3@citrix.com> wrote:
>>> On 18/12/14 18:27, Roger Pau Monne wrote:
>>>> Prevent Dom0 from accessing HPET MMIO region by adding it to the list of
>>>> denied memory regions.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Apologies that this reply is split between patch 0 and 2 - I replied to
>>> your cover letter before reading this patch.
>>>
>>> Denying access is only valid if acpi_table_hpet.flags & 
>>> ACPI_HPET_PAGE_PROTECT4 is true.
>> Having just checked (as an example) the most modern Intel box I
>> have direct access to, I wonder how many systems actually supply
>> other than 0 here. Perhaps we ought to at once add a command
>> line option to trigger the denial?
> 
> I also can't find a server which sets this flag.  I wonder how many
> systems actually have other things sitting in the remainder of the page.

One would think (or should I say hope) that there's at least nothing
with read side effects anywhere, or else Linux'es exposing of the
page to user mode would be a security problem. Perhaps we should
also limit Dom0 mappings to r/o when we can't hide the page
altogether.

Jan

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