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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxx>
> ---
>   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