Re: [PATCH] vpci: add permission checks to map_range()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2023 17:37, Roger Pau Monne wrote:
> @@ -1184,6 +1177,20 @@ int __init dom0_construct_pvh(struct domain *d, const 
> module_t *image,
>  
>      printk(XENLOG_INFO "*** Building a PVH Dom%d ***\n", d->domain_id);
>  
> +    if ( is_hardware_domain(d) )
> +    {
> +        /*
> +         * Setup permissions early so that calls to add MMIO regions to the
> +         * p2m as part of vPCI setup don't fail due to permission checks.
> +         */
> +        rc = dom0_setup_permissions(d);
> +        if ( rc )
> +        {
> +            printk("%pd unable to setup permissions: %d\n", d, rc);

The switch from panic() to printk() may want mentioning in the description
as deliberate. (The usefulness of %pd here is debatable, as it can't be
other than Dom0. But I don't mind.)

> @@ -43,6 +46,21 @@ static int cf_check map_range(
>      {
>          unsigned long size = e - s + 1;
>  
> +        if ( !iomem_access_permitted(map->d, s, e) )
> +        {
> +            gprintk(XENLOG_WARNING,
> +                    "%pd denied access to MMIO range [%#lx, %#lx]\n", s, e);

This doesn't look like it would compile. Also gprintk() logs current,
which I'm not sure is generally applicable here. IOW I think it wants
to be

            printk(XENLOG_G_WARNING,
                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
                   map->d, s, e);

Same for the other log message then.

Another Dom0 related concern can probably be put off until we actually
get a report of this failing (which may be more likely because of the
XSM check below): The function being used as a callback passed to
rangeset_consume_ranges(), failure may affect just a single BAR, while
the incoming range may cover multiple of them in one go. Depending on
what functionality such a BAR covers, the device may remain usable (a
typical example of what I'm thinking of is a multi-function device
having serial and/or parallel port on it, which are fine to be driven
via I/O ports even if driving via MMIO is possible [and would likely
be more efficient]). Of course, to allow some MMIO bars to be used
while prohibiting use of some others, further trickery may be needed.
But not exposing the device to Dom0 at all doesn't seem very nice in
such a case.

Jan

> +            return -EPERM;
> +        }
> +
> +        rc = xsm_iomem_mapping(XSM_HOOK, map->d, s, e, map->map);
> +        if ( rc )
> +        {
> +            gprintk(XENLOG_WARNING,
> +                    "%pd XSM denied access to MMIO range [%#lx, %#lx]\n", s, 
> e);
> +            return rc;
> +        }
> +
>          /*
>           * ARM TODOs:
>           * - On ARM whether the memory is prefetchable or not should be 
> passed