c/s 24425:053a44894279 (xsm: add checks on PCI configuration access)

c/s 24425:053a44894279 (xsm: add checks on PCI configuration access)

[Jan Beulich]

The mmconfig part of this is seriously broken: These operations,
even when carried out by Dom0, are MMIO accesses, and hence
are invisible to the hypervisor without extra handling. Putting
the checks into pci_mmcfg_{read,write}() has the effect of
potentially denying the _hypervisor_ access. So I think at least
that part needs to be reverted.

Even the I/O port base logic isn't fully correct - AMD's extension
to access extended config space isn't being taken care of (i.e.
wrong register values might get passed to the xsm callback).

(It is, btw, also this c/s that prompted the fix titled "x86/PCI:
fix guest_io_read() when pci_cfg_ok() denies access" I sent
out earlier today, so if we decide to revert the whole c/s, that
wouldn't be needed anymore. Yet the function comes handy
for dealing with the MMIO-write-masking that we're currently
evaluating with the AMD folks to get their IOMMU interrupts
working again with recent Linux Dom0 - see yesterday's
http://lists.xen.org/archives/html/xen-devel/2012-06/msg01129.html.)

Jan

Re: c/s 24425:053a44894279 (xsm: add checks on PCI configuration access)

[Daniel De Graaf]

On 06/21/2012 09:20 AM, Jan Beulich wrote:
> The mmconfig part of this is seriously broken: These operations,
> even when carried out by Dom0, are MMIO accesses, and hence
> are invisible to the hypervisor without extra handling. Putting
> the checks into pci_mmcfg_{read,write}() has the effect of
> potentially denying the _hypervisor_ access. So I think at least
> that part needs to be reverted.

I agree - the XSM checks are intended to be done only when the hypervisor
is accessing on behalf of the domain, which looks to be covered by the
traps part of the patch. These checks are currently intended to deny a
domain with IS_PRIV but without full hardware access - in particular,
without access to the PCI configuration MMIO area - from using legacy 
register access to reconfigure PCI devices.

While it may be useful to extend this access check to include the PCI 
configuration MMIO pages, this would require emulating both reads and
writes to any page that has entries that a particular domain does not
have access to. The existing pciback/pcifront configuration access model
already handles these issues without changes to the hypervisor.

> Even the I/O port base logic isn't fully correct - AMD's extension
> to access extended config space isn't being taken care of (i.e.
> wrong register values might get passed to the xsm callback).

Looks like a trivial one-line fix, I'll send a patch momentarily. I wasn't
aware of that extension when I was writing the cf8 decoding.

[PATCH] x86/PCI: pass correct register value to XSM

[Daniel De Graaf]

When using AMD's extension to access the extended PCI config space, only
the low byte of the register number was being passed to XSM. Include the
full value.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/arch/x86/traps.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 2264583..38ad9e7 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1691,6 +1691,7 @@ static int pci_cfg_ok(struct domain *d, int write, int size)
 
     machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
     start = d->arch.pci_cf8 & 0xFF;
+    start |= (d->arch.pci_cf8 >> 16) & 0xF00;
     end = start + size - 1;
     if (xsm_pci_config_permission(d, machine_bdf, start, end, write))
         return 0;
-- 
1.7.10.2

Re: [PATCH] x86/PCI: pass correct register value to XSM

[Jan Beulich]

>>> On 21.06.12 at 16:20, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> When using AMD's extension to access the extended PCI config space, only
> the low byte of the register number was being passed to XSM. Include the
> full value.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  xen/arch/x86/traps.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 2264583..38ad9e7 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1691,6 +1691,7 @@ static int pci_cfg_ok(struct domain *d, int write, int 
> size)
>  
>      machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
>      start = d->arch.pci_cf8 & 0xFF;
> +    start |= (d->arch.pci_cf8 >> 16) & 0xF00;

You should be doing this only if the functionality is actually
available (AMD only) and enabled (MSR bit). See Linux'es
pci_io_ecs_init() and Xen's handling of Dom0 writes to
MSR_AMD64_NB_CFG.

Jan

>      end = start + size - 1;
>      if (xsm_pci_config_permission(d, machine_bdf, start, end, write))
>          return 0;
> -- 
> 1.7.10.2

[PATCH] x86/PCI: pass correct register value to XSM

[Daniel De Graaf]

When attempting to use AMD's extension to access the extended PCI config
space, only the low byte of the register number was being passed to XSM.
Include the correct value of the register if this feature is enabled;
otherwise, bits 24-30 of port cf8 are reserved, so disallow the invalid
access.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/arch/x86/traps.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 2264583..d836452 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1686,11 +1686,24 @@ static int pci_cfg_ok(struct domain *d, int write, int size)
 {
     uint32_t machine_bdf;
     uint16_t start, end;
+    uint64_t msr_val;
     if (!IS_PRIV(d))
         return 0;
 
     machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
     start = d->arch.pci_cf8 & 0xFF;
+    if ( d->arch.pci_cf8 & 0x0F000000 )
+    {
+        /* Possible AMD extended configuration access */
+        if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
+             boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 )
+            return 0;
+        if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) )
+            return 0;
+        if ( !(msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
+            return 0;
+        start |= (d->arch.pci_cf8 >> 16) & 0xF00;
+    }
     end = start + size - 1;
     if (xsm_pci_config_permission(d, machine_bdf, start, end, write))
         return 0;
-- 
1.7.10.2

Re: [PATCH] x86/PCI: pass correct register value to XSM

[Jan Beulich]

>>> On 21.06.12 at 19:01, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> When attempting to use AMD's extension to access the extended PCI config
> space, only the low byte of the register number was being passed to XSM.
> Include the correct value of the register if this feature is enabled;
> otherwise, bits 24-30 of port cf8 are reserved, so disallow the invalid
> access.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  xen/arch/x86/traps.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 2264583..d836452 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1686,11 +1686,24 @@ static int pci_cfg_ok(struct domain *d, int write, 
> int size)
>  {
>      uint32_t machine_bdf;
>      uint16_t start, end;
> +    uint64_t msr_val;
>      if (!IS_PRIV(d))
>          return 0;
>  
>      machine_bdf = (d->arch.pci_cf8 >> 8) & 0xFFFF;
>      start = d->arch.pci_cf8 & 0xFF;
> +    if ( d->arch.pci_cf8 & 0x0F000000 )
> +    {
> +        /* Possible AMD extended configuration access */
> +        if ( boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
> +             boot_cpu_data.x86 < 0x10 || boot_cpu_data.x86 > 0x17 )
> +            return 0;
> +        if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) )
> +            return 0;
> +        if ( !(msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
> +            return 0;

Not quite, still. Let me come back with a modified version later
today.

Jan

> +        start |= (d->arch.pci_cf8 >> 16) & 0xF00;
> +    }
>      end = start + size - 1;
>      if (xsm_pci_config_permission(d, machine_bdf, start, end, write))
>          return 0;
> -- 
> 1.7.10.2

[PATCH] x86/PCI: pass correct register value to XSM

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

When attempting to use AMD's extension to access the extended PCI config
space, only the low byte of the register number was being passed to XSM.
Include the correct value of the register if this feature is enabled;
otherwise, bits 24-30 of port cf8 are reserved, so disallow the invalid
access.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Don't fail the permission check except when the MSR can't be read.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1701,6 +1701,18 @@ static int pci_cfg_ok(struct domain *d, 
             return 0;
     }
     start = d->arch.pci_cf8 & 0xFF;
+    /* AMD extended configuration space access? */
+    if ( (d->arch.pci_cf8 & 0x0F000000) &&
+         boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
+         boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
+    {
+        uint64_t msr_val;
+
+        if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) )
+            return 0;
+        if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) )
+            start |= (d->arch.pci_cf8 >> 16) & 0xF00;
+    }
     end = start + size - 1;
     if (xsm_pci_config_permission(d, machine_bdf, start, end, write))
         return 0;




[-- Attachment #2: x86-PCI-xsm-AMD-ECS.patch --]
[-- Type: text/plain, Size: 1315 bytes --]

x86/PCI: pass correct register value to XSM

When attempting to use AMD's extension to access the extended PCI config
space, only the low byte of the register number was being passed to XSM.
Include the correct value of the register if this feature is enabled;
otherwise, bits 24-30 of port cf8 are reserved, so disallow the invalid
access.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

Don't fail the permission check except when the MSR can't be read.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1701,6 +1701,18 @@ static int pci_cfg_ok(struct domain *d, 
             return 0;
     }
     start = d->arch.pci_cf8 & 0xFF;
+    /* AMD extended configuration space access? */
+    if ( (d->arch.pci_cf8 & 0x0F000000) &&
+         boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
+         boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
+    {
+        uint64_t msr_val;
+
+        if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) )
+            return 0;
+        if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) )
+            start |= (d->arch.pci_cf8 >> 16) & 0xF00;
+    }
     end = start + size - 1;
     if (xsm_pci_config_permission(d, machine_bdf, start, end, write))
         return 0;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] x86/PCI: pass correct register value to XSM

[Keir Fraser]

On 22/06/2012 09:33, "Jan Beulich" <JBeulich@suse.com> wrote:

> When attempting to use AMD's extension to access the extended PCI config
> space, only the low byte of the register number was being passed to XSM.
> Include the correct value of the register if this feature is enabled;
> otherwise, bits 24-30 of port cf8 are reserved, so disallow the invalid
> access.
> 
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> 
> Don't fail the permission check except when the MSR can't be read.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Keir Fraser <keir@xen.org>

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1701,6 +1701,18 @@ static int pci_cfg_ok(struct domain *d,
>              return 0;
>      }
>      start = d->arch.pci_cf8 & 0xFF;
> +    /* AMD extended configuration space access? */
> +    if ( (d->arch.pci_cf8 & 0x0F000000) &&
> +         boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
> +         boot_cpu_data.x86 >= 0x10 && boot_cpu_data.x86 <= 0x17 )
> +    {
> +        uint64_t msr_val;
> +
> +        if ( rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) )
> +            return 0;
> +        if ( msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT) )
> +            start |= (d->arch.pci_cf8 >> 16) & 0xF00;
> +    }
>      end = start + size - 1;
>      if (xsm_pci_config_permission(d, machine_bdf, start, end, write))
>          return 0;
> 
> 
> 

Re: c/s 24425:053a44894279 (xsm: add checks on PCI configuration access)

[Jan Beulich]

>>> On 21.06.12 at 16:19, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> On 06/21/2012 09:20 AM, Jan Beulich wrote:
>> The mmconfig part of this is seriously broken: These operations,
>> even when carried out by Dom0, are MMIO accesses, and hence
>> are invisible to the hypervisor without extra handling. Putting
>> the checks into pci_mmcfg_{read,write}() has the effect of
>> potentially denying the _hypervisor_ access. So I think at least
>> that part needs to be reverted.
> 
> I agree - the XSM checks are intended to be done only when the hypervisor
> is accessing on behalf of the domain, which looks to be covered by the
> traps part of the patch. These checks are currently intended to deny a
> domain with IS_PRIV but without full hardware access - in particular,
> without access to the PCI configuration MMIO area - from using legacy 
> register access to reconfigure PCI devices.
> 
> While it may be useful to extend this access check to include the PCI 
> configuration MMIO pages, this would require emulating both reads and
> writes to any page that has entries that a particular domain does not
> have access to. The existing pciback/pcifront configuration access model
> already handles these issues without changes to the hypervisor.

So do I read correctly that you agree to revert that part of said
c/s?

Jan

Re: c/s 24425:053a44894279 (xsm: add checks on PCI configuration access)

[Daniel De Graaf]

On 06/21/2012 11:13 AM, Jan Beulich wrote:
>>>> On 21.06.12 at 16:19, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> On 06/21/2012 09:20 AM, Jan Beulich wrote:
>>> The mmconfig part of this is seriously broken: These operations,
>>> even when carried out by Dom0, are MMIO accesses, and hence
>>> are invisible to the hypervisor without extra handling. Putting
>>> the checks into pci_mmcfg_{read,write}() has the effect of
>>> potentially denying the _hypervisor_ access. So I think at least
>>> that part needs to be reverted.
>>
>> I agree - the XSM checks are intended to be done only when the hypervisor
>> is accessing on behalf of the domain, which looks to be covered by the
>> traps part of the patch. These checks are currently intended to deny a
>> domain with IS_PRIV but without full hardware access - in particular,
>> without access to the PCI configuration MMIO area - from using legacy 
>> register access to reconfigure PCI devices.
>>
>> While it may be useful to extend this access check to include the PCI 
>> configuration MMIO pages, this would require emulating both reads and
>> writes to any page that has entries that a particular domain does not
>> have access to. The existing pciback/pcifront configuration access model
>> already handles these issues without changes to the hypervisor.
> 
> So do I read correctly that you agree to revert that part of said
> c/s?
> 
> Jan
> 

Yes.

-- 
Daniel De Graaf
National Security Agency