Just like it's done for the XEN_DOMCTL_memory_mapping hypercall, add
the permissions checks to vPCI map_range(), which is used to map the
BARs into the domain p2m.

Adding those checks requires that for x86 PVH hardware domain builder
the permissions are set before initializing the IOMMU, or else
attempts to initialize vPCI done as part of IOMMU device setup will
fail due to missing permissions to create the BAR mappings.

While moving the call to dom0_setup_permissions() convert the panic()
used for error handling to a printk, the caller will already panic if
required.

Fixes: 9c244fdef7e7 ('vpci: add header handlers')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Fix printk calls.
 - Expand commit message.
---
I'm unsure whether on ARM MMIO permissions are properly set for the
hardware domain, but I don't have a system to test with.
---
 xen/arch/x86/hvm/dom0_build.c | 21 ++++++++++++++-------
 xen/drivers/vpci/header.c     | 20 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index a7ae9c3b046e..bc0e290db612 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -715,13 +715,6 @@ static int __init pvh_setup_cpus(struct domain *d, paddr_t entry,
         return rc;
     }
 
-    rc = dom0_setup_permissions(d);
-    if ( rc )
-    {
-        panic("Unable to setup Dom0 permissions: %d\n", rc);
-        return rc;
-    }
-
     update_domain_wallclock_time(d);
 
     v->is_initialised = 1;
@@ -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);
+            return rc;
+        }
+    }
+
     /*
      * NB: MMCFG initialization needs to be performed before iommu
      * initialization so the iommu code can fetch the MMCFG regions used by the
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index b41556d00746..60f7049e3498 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -17,10 +17,13 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <xen/iocap.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
 #include <xen/vpci.h>
 
+#include <xsm/xsm.h>
+
 #include <asm/event.h>
 #include <asm/p2m.h>
 
@@ -43,6 +46,23 @@ static int cf_check map_range(
     {
         unsigned long size = e - s + 1;
 
+        if ( !iomem_access_permitted(map->d, s, e) )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
+                   map->d, s, e);
+            return -EPERM;
+        }
+
+        rc = xsm_iomem_mapping(XSM_HOOK, map->d, s, e, map->map);
+        if ( rc )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
+                   map->d, s, e, rc);
+            return rc;
+        }
+
         /*
          * ARM TODOs:
          * - On ARM whether the memory is prefetchable or not should be passed
-- 
2.41.0

On 26.07.2023 16:01, Roger Pau Monne wrote:
> Just like it's done for the XEN_DOMCTL_memory_mapping hypercall, add
> the permissions checks to vPCI map_range(), which is used to map the
> BARs into the domain p2m.
> 
> Adding those checks requires that for x86 PVH hardware domain builder
> the permissions are set before initializing the IOMMU, or else
> attempts to initialize vPCI done as part of IOMMU device setup will
> fail due to missing permissions to create the BAR mappings.
> 
> While moving the call to dom0_setup_permissions() convert the panic()
> used for error handling to a printk, the caller will already panic if
> required.
> 
> Fixes: 9c244fdef7e7 ('vpci: add header handlers')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I've committed this, but despite the Fixes: tag I'm not sure this
wants backporting. Thoughts?

Jan

On 7/27/23 03:56, Jan Beulich wrote:
> On 26.07.2023 16:01, Roger Pau Monne wrote:
>> Just like it's done for the XEN_DOMCTL_memory_mapping hypercall, add
>> the permissions checks to vPCI map_range(), which is used to map the
>> BARs into the domain p2m.
>>
>> Adding those checks requires that for x86 PVH hardware domain builder
>> the permissions are set before initializing the IOMMU, or else
>> attempts to initialize vPCI done as part of IOMMU device setup will
>> fail due to missing permissions to create the BAR mappings.
>>
>> While moving the call to dom0_setup_permissions() convert the panic()
>> used for error handling to a printk, the caller will already panic if
>> required.
>>
>> Fixes: 9c244fdef7e7 ('vpci: add header handlers')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I've committed this, but despite the Fixes: tag I'm not sure this
> wants backporting. Thoughts?
> 
> Jan

 From a cursory review thus far, since this introduced a new XSM hook 
site, shouldn't this have at least had an Rb by an XSM 
reviewer/maintainer? I would have replied sooner, but have been on 
holiday for last two weeks.

V/r,
Daniel P. Smith

On 27.07.2023 13:07, Daniel P. Smith wrote:
> 
> 
> On 7/27/23 03:56, Jan Beulich wrote:
>> On 26.07.2023 16:01, Roger Pau Monne wrote:
>>> Just like it's done for the XEN_DOMCTL_memory_mapping hypercall, add
>>> the permissions checks to vPCI map_range(), which is used to map the
>>> BARs into the domain p2m.
>>>
>>> Adding those checks requires that for x86 PVH hardware domain builder
>>> the permissions are set before initializing the IOMMU, or else
>>> attempts to initialize vPCI done as part of IOMMU device setup will
>>> fail due to missing permissions to create the BAR mappings.
>>>
>>> While moving the call to dom0_setup_permissions() convert the panic()
>>> used for error handling to a printk, the caller will already panic if
>>> required.
>>>
>>> Fixes: 9c244fdef7e7 ('vpci: add header handlers')
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> I've committed this, but despite the Fixes: tag I'm not sure this
>> wants backporting. Thoughts?
> 
>  From a cursory review thus far, since this introduced a new XSM hook 
> site, shouldn't this have at least had an Rb by an XSM 
> reviewer/maintainer?

Probably, but already back then I said this model isn't going to work
flawlessly.

> I would have replied sooner, but have been on holiday for last two weeks.

I guess there was no way for us to know without you sending a note to
private@ (which, I will admit, you may not even have been aware of).

Jan

On Thu, Jul 27, 2023 at 09:56:08AM +0200, Jan Beulich wrote:
> On 26.07.2023 16:01, Roger Pau Monne wrote:
> > Just like it's done for the XEN_DOMCTL_memory_mapping hypercall, add
> > the permissions checks to vPCI map_range(), which is used to map the
> > BARs into the domain p2m.
> > 
> > Adding those checks requires that for x86 PVH hardware domain builder
> > the permissions are set before initializing the IOMMU, or else
> > attempts to initialize vPCI done as part of IOMMU device setup will
> > fail due to missing permissions to create the BAR mappings.
> > 
> > While moving the call to dom0_setup_permissions() convert the panic()
> > used for error handling to a printk, the caller will already panic if
> > required.
> > 
> > Fixes: 9c244fdef7e7 ('vpci: add header handlers')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I've committed this, but despite the Fixes: tag I'm not sure this
> wants backporting. Thoughts?

It was IMO an omission from that commit, however given vPCI so far is
only used by dom0 (an in experimental mode) I don't see much reason to
backport it.

Thanks, Roger.

Hi Roger,

> On 26 Jul 2023, at 3:01 pm, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> Just like it's done for the XEN_DOMCTL_memory_mapping hypercall, add
> the permissions checks to vPCI map_range(), which is used to map the
> BARs into the domain p2m.
> 
> Adding those checks requires that for x86 PVH hardware domain builder
> the permissions are set before initializing the IOMMU, or else
> attempts to initialize vPCI done as part of IOMMU device setup will
> fail due to missing permissions to create the BAR mappings.
> 
> While moving the call to dom0_setup_permissions() convert the panic()
> used for error handling to a printk, the caller will already panic if
> required.
> 
> Fixes: 9c244fdef7e7 ('vpci: add header handlers')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I tested the patch on ARM board with vPCI enabled everything works.

Reviewed-by: Rahul Singh <rahul.singh@arm.com>
Tested-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul

On 26.07.2023 16:01, Roger Pau Monne wrote:
> Just like it's done for the XEN_DOMCTL_memory_mapping hypercall, add
> the permissions checks to vPCI map_range(), which is used to map the
> BARs into the domain p2m.
> 
> Adding those checks requires that for x86 PVH hardware domain builder
> the permissions are set before initializing the IOMMU, or else
> attempts to initialize vPCI done as part of IOMMU device setup will
> fail due to missing permissions to create the BAR mappings.
> 
> While moving the call to dom0_setup_permissions() convert the panic()
> used for error handling to a printk, the caller will already panic if
> required.
> 
> Fixes: 9c244fdef7e7 ('vpci: add header handlers')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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