[PATCH] arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0

[PATCH] arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0

[Rahul Singh]

Xen should control the SMMUv3 devices therefore, don't expose the
SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for
dom0 and also make ACPI IORT SMMUv3 node type to 0xff.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
index bbdc90f92c..ec0b5b261f 100644
--- a/xen/arch/arm/acpi/domain_build.c
+++ b/xen/arch/arm/acpi/domain_build.c
@@ -14,6 +14,7 @@
 #include <xen/acpi.h>
 #include <xen/event.h>
 #include <xen/iocap.h>
+#include <xen/sizes.h>
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <acpi/actables.h>
@@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d)
 {
     acpi_status status;
     struct acpi_table_spcr *spcr = NULL;
+    struct acpi_table_iort *iort;
     unsigned long mfn;
     int rc;
 
@@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d)
         printk("Failed to get SPCR table, Xen console may be unavailable\n");
     }
 
+    status = acpi_get_table(ACPI_SIG_IORT, 0,
+                            (struct acpi_table_header **)&iort);
+
+    if ( ACPI_SUCCESS(status) )
+    {
+        int i;
+        struct acpi_iort_node *node, *end;
+        node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset);
+        end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length);
+
+        for ( i = 0; i < iort->node_count; i++ )
+        {
+            if ( node >= end )
+                break;
+
+            switch ( node->type )
+            {
+                case ACPI_IORT_NODE_SMMU_V3:
+                {
+                    struct acpi_iort_smmu_v3 *smmu;
+                    smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
+                    mfn = paddr_to_pfn(smmu->base_address);
+                    rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K));
+                    if ( rc )
+                        printk("iomem_deny_access failed for SMMUv3\n");
+                    node->type = 0xff;
+                    break;
+                }
+            }
+            node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length);
+        }
+    }
+    else
+    {
+        printk("Failed to get IORT table\n");
+        return -EINVAL;
+    }
+
     /* Deny MMIO access for GIC regions */
     return gic_iomem_deny_access(d);
 }
-- 
2.25.1

Re: [PATCH] arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0

[Julien Grall]

Hi Rahul,

On 27/04/2022 17:12, Rahul Singh wrote:
> Xen should control the SMMUv3 devices therefore, don't expose the
> SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for
> dom0 and also make ACPI IORT SMMUv3 node type to 0xff.

Looking at the IORT spec (ARM DEN 0049E), 255 (0xff) is marked as 
reserved. So I don't think we can "allocate" 0xff to mean invalid 
without updating the spec. Did you engage with whoever own the spec?

> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
> index bbdc90f92c..ec0b5b261f 100644
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -14,6 +14,7 @@
>   #include <xen/acpi.h>
>   #include <xen/event.h>
>   #include <xen/iocap.h>
> +#include <xen/sizes.h>
>   #include <xen/device_tree.h>
>   #include <xen/libfdt/libfdt.h>
>   #include <acpi/actables.h>
> @@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>   {
>       acpi_status status;
>       struct acpi_table_spcr *spcr = NULL;
> +    struct acpi_table_iort *iort;
>       unsigned long mfn;
>       int rc;
>   
> @@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>           printk("Failed to get SPCR table, Xen console may be unavailable\n");
>       }
>   
> +    status = acpi_get_table(ACPI_SIG_IORT, 0,
> +                            (struct acpi_table_header **)&iort);

At some point we will need to add support to hide the ARM SMMU device 
and possibly some devices. So I think it would be better to create a 
function that would deal with the IORT.

> +
> +    if ( ACPI_SUCCESS(status) )
> +    {
> +        int i;

Please use unsigned int.

> +        struct acpi_iort_node *node, *end;

Coding style: Please add a newline.

> +        node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset);
> +        end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length);
> +
> +        for ( i = 0; i < iort->node_count; i++ )
> +        {
> +            if ( node >= end )

Wouldn't this only happen if the table is somehow corrupted? If so, I 
think we should print an error (or even panic).

> +                break;
> +
> +            switch ( node->type )
> +            {
> +                case ACPI_IORT_NODE_SMMU_V3:

Coding style: The keyword "case" should be aligned the the start of the 
keyword "switch".

> +                {
> +                    struct acpi_iort_smmu_v3 *smmu;

Coding style: Newline.

> +                    smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> +                    mfn = paddr_to_pfn(smmu->base_address);
> +                    rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K));
> +                    if ( rc )
> +                        printk("iomem_deny_access failed for SMMUv3\n");
> +                    node->type = 0xff;

'node' points to the Xen copy of the ACPI table. We should really not 
touch this copy. Instead, we should modify the version that will be used 
by dom0.

Furthermore, if we go down the road to update node->type, we should 0 
the node to avoid leaking the information to dom0.

> +                    break;
> +                }
> +            }
> +            node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length);
> +        }
> +    }
> +    else
> +    {
> +        printk("Failed to get IORT table\n");
> +        return -EINVAL;
> +    }

The IORT is not yet parsed by Xen and AFAIK is optional. So I don't 
think we should return an error.

> +
>       /* Deny MMIO access for GIC regions */
>       return gic_iomem_deny_access(d);
>   }

Cheers,

-- 
Julien Grall

Re: [PATCH] arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0

[Rahul Singh]

Hi Julien,

> On 27 Apr 2022, at 7:26 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 27/04/2022 17:12, Rahul Singh wrote:
>> Xen should control the SMMUv3 devices therefore, don't expose the
>> SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for
>> dom0 and also make ACPI IORT SMMUv3 node type to 0xff.
> 
> Looking at the IORT spec (ARM DEN 0049E), 255 (0xff) is marked as reserved. So I don't think we can "allocate" 0xff to mean invalid without updating the spec. Did you engage with whoever own the spec?

Yes I agree with you 0xff is reserved for future use. I didn’t find any other value to make node invalid. 
Linux kernel is mostly using the node->type to process the SMMUv3 or other IORT node so I thought this is the only possible solution to hide SMMUv3 for dom0

If you have any other suggestion to hide the SMMUv3 node I am okay to use that.
> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++
>> 1 file changed, 40 insertions(+)
>> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
>> index bbdc90f92c..ec0b5b261f 100644
>> --- a/xen/arch/arm/acpi/domain_build.c
>> +++ b/xen/arch/arm/acpi/domain_build.c
>> @@ -14,6 +14,7 @@
>> #include <xen/acpi.h>
>> #include <xen/event.h>
>> #include <xen/iocap.h>
>> +#include <xen/sizes.h>
>> #include <xen/device_tree.h>
>> #include <xen/libfdt/libfdt.h>
>> #include <acpi/actables.h>
>> @@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>> {
>> acpi_status status;
>> struct acpi_table_spcr *spcr = NULL;
>> + struct acpi_table_iort *iort;
>> unsigned long mfn;
>> int rc;
>> @@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>> printk("Failed to get SPCR table, Xen console may be unavailable\n");
>> }
>> + status = acpi_get_table(ACPI_SIG_IORT, 0,
>> + (struct acpi_table_header **)&iort);
> 
> At some point we will need to add support to hide the ARM SMMU device and possibly some devices. So I think it would be better to create a function that would deal with the IORT.

Ok. Let me add the function in next version.
> 
>> +
>> + if ( ACPI_SUCCESS(status) )
>> + {
>> + int i;
> 
> Please use unsigned int.
Ack.
> 
>> + struct acpi_iort_node *node, *end;
> 
> Coding style: Please add a newline.

Ack. 
> 
>> + node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset);
>> + end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length);
>> +
>> + for ( i = 0; i < iort->node_count; i++ )
>> + {
>> + if ( node >= end )
> 
> Wouldn't this only happen if the table is somehow corrupted? If so, I think we should print an error (or even panic).

Ok.
> 
>> + break;
>> +
>> + switch ( node->type )
>> + {
>> + case ACPI_IORT_NODE_SMMU_V3:
> 
> Coding style: The keyword "case" should be aligned the the start of the keyword "switch”.
Ack. 
> 
>> + {
>> + struct acpi_iort_smmu_v3 *smmu;
> 
> Coding style: Newline.

Ack. 
>> + smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>> + mfn = paddr_to_pfn(smmu->base_address);
>> + rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K));
>> + if ( rc )
>> + printk("iomem_deny_access failed for SMMUv3\n");
>> + node->type = 0xff;
> 
> 'node' points to the Xen copy of the ACPI table. We should really not touch this copy. Instead, we should modify the version that will be used by dom0.

As of now IORT is untouched by Xen and mapped to dom0. I will create the IORT table for dom0 and modify the node SMMUv3 that will be used by dom0.
> 
> Furthermore, if we go down the road to update node->type, we should 0 the node to avoid leaking the information to dom0.

I am not sure if we can zero the node, let me check and come back to you. 
> 
>> + break;
>> + }
>> + }
>> + node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length);
>> + }
>> + }
>> + else
>> + {
>> + printk("Failed to get IORT table\n");
>> + return -EINVAL;
>> + }
> 
> The IORT is not yet parsed by Xen and AFAIK is optional. So I don't think we should return an error.

Ack. 

Regards,
Rahul

Re: [PATCH] arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0

[Julien Grall]

On 29/04/2022 19:18, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

>> On 27 Apr 2022, at 7:26 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Rahul,
>>
>> On 27/04/2022 17:12, Rahul Singh wrote:
>>> Xen should control the SMMUv3 devices therefore, don't expose the
>>> SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for
>>> dom0 and also make ACPI IORT SMMUv3 node type to 0xff.
>>
>> Looking at the IORT spec (ARM DEN 0049E), 255 (0xff) is marked as reserved. So I don't think we can "allocate" 0xff to mean invalid without updating the spec. Did you engage with whoever own the spec?
> 
> Yes I agree with you 0xff is reserved for future use. I didn’t find any other value to make node invalid.
> Linux kernel is mostly using the node->type to process the SMMUv3 or other IORT node so I thought this is the only possible solution to hide SMMUv3 for dom0
> If you have any other suggestion to hide the SMMUv3 node I am okay to use that.
The other solution is to remove completely the SMMUv3 node from the 
IORT. This would require more work as you would need to fully rewrite 
the IORT.

Hence why I suggested to speak with the spec owner (it seems to be Arm) 
to reserve 0xff as "Invalid/Ignore".

>>> + smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
>>> + mfn = paddr_to_pfn(smmu->base_address);
>>> + rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K));
>>> + if ( rc )
>>> + printk("iomem_deny_access failed for SMMUv3\n");
>>> + node->type = 0xff;
>>
>> 'node' points to the Xen copy of the ACPI table. We should really not touch this copy. Instead, we should modify the version that will be used by dom0.
> 
> As of now IORT is untouched by Xen and mapped to dom0. I will create the IORT table for dom0 and modify the node SMMUv3 that will be used by dom0.
>>
>> Furthermore, if we go down the road to update node->type, we should 0 the node to avoid leaking the information to dom0.
> 
> I am not sure if we can zero the node, let me check and come back to you.

By writing node->type, you already invalidate the content because the 
software cannot know how to interpret it. At which point, zeroing it 
should make no difference for software parsing the table afterwards. 
This may be a problem for software parsing before hand and keeping a 
pointer to the entry. But then, this is yet another reason to no updated 
the host IORT and create a copy for dom0.

Cheers,

-- 
Julien Grall

Re: [PATCH] arm/acpi: don't expose the ACPI IORT SMMUv3 entry to dom0

[Robin Murphy]

On 2022-04-27 17:12, Rahul Singh wrote:
> Xen should control the SMMUv3 devices therefore, don't expose the
> SMMUv3 devices to dom0. Deny iomem access to SMMUv3 address space for
> dom0 and also make ACPI IORT SMMUv3 node type to 0xff.

...making the resulting IORT technically useless to consumers. ID 
mappings for all the Root Complex, Named Component and RMR nodes which 
were supposed to be translated through that SMMU node are now invalid, 
because ID mappings can only target an SMMU or ITS node. I can't guess 
at how other consumers may react, but Linux's IORT code is going to 
experience undefined behaviour when tries to translate any MSI DeviceID 
mappings through this invalid node, since it uses a bitmap of (1 << 
node->type) internally; beyond that we're not as strict as we could be 
and make some pretty loose assumptions about what we're parsing, so it 
might even appear to work, but that could easily change at any time in 
future and is absolutely not something Xen or any other software should 
try to rely on.

Thanks,
Robin.

> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   xen/arch/arm/acpi/domain_build.c | 40 ++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
> 
> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
> index bbdc90f92c..ec0b5b261f 100644
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -14,6 +14,7 @@
>   #include <xen/acpi.h>
>   #include <xen/event.h>
>   #include <xen/iocap.h>
> +#include <xen/sizes.h>
>   #include <xen/device_tree.h>
>   #include <xen/libfdt/libfdt.h>
>   #include <acpi/actables.h>
> @@ -30,6 +31,7 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>   {
>       acpi_status status;
>       struct acpi_table_spcr *spcr = NULL;
> +    struct acpi_table_iort *iort;
>       unsigned long mfn;
>       int rc;
>   
> @@ -55,6 +57,44 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>           printk("Failed to get SPCR table, Xen console may be unavailable\n");
>       }
>   
> +    status = acpi_get_table(ACPI_SIG_IORT, 0,
> +                            (struct acpi_table_header **)&iort);
> +
> +    if ( ACPI_SUCCESS(status) )
> +    {
> +        int i;
> +        struct acpi_iort_node *node, *end;
> +        node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset);
> +        end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length);
> +
> +        for ( i = 0; i < iort->node_count; i++ )
> +        {
> +            if ( node >= end )
> +                break;
> +
> +            switch ( node->type )
> +            {
> +                case ACPI_IORT_NODE_SMMU_V3:
> +                {
> +                    struct acpi_iort_smmu_v3 *smmu;
> +                    smmu = (struct acpi_iort_smmu_v3 *)node->node_data;
> +                    mfn = paddr_to_pfn(smmu->base_address);
> +                    rc = iomem_deny_access(d, mfn, mfn + PFN_UP(SZ_128K));
> +                    if ( rc )
> +                        printk("iomem_deny_access failed for SMMUv3\n");
> +                    node->type = 0xff;
> +                    break;
> +                }
> +            }
> +            node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->length);
> +        }
> +    }
> +    else
> +    {
> +        printk("Failed to get IORT table\n");
> +        return -EINVAL;
> +    }
> +
>       /* Deny MMIO access for GIC regions */
>       return gic_iomem_deny_access(d);
>   }