| Message ID | 20230724153741.42374-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | vpci: add permission checks to map_range() | expand |
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
On Wed, Jul 26, 2023 at 02:36:17PM +0200, Jan Beulich wrote: > 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.) The printk just above uses Dom%d, so I assumed it was best to not hardcode 0 here either. > > @@ -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. Oh great. I changed the format of those before sending and didn't rebuild properly. > 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. Hm, I see. For dom0 we might want to consider ignoring mapping failures, the problem is that we would need to narrow down the pages not allowed to be mapped, as part of the range passed to map_range() might be allowed. We would need to resort to checking permissions on a page by page basis, which is not overly nice. I think it's more likely for such BARs to be marked as read-only (instead of denying access), in which case the checking here would still be OK. Thanks, Roger.
On 26.07.2023 15:37, Roger Pau Monné wrote: > On Wed, Jul 26, 2023 at 02:36:17PM +0200, Jan Beulich wrote: >> 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. > > Hm, I see. For dom0 we might want to consider ignoring mapping > failures, the problem is that we would need to narrow down the pages > not allowed to be mapped, as part of the range passed to map_range() > might be allowed. We would need to resort to checking permissions on > a page by page basis, which is not overly nice. Right, all of which is why I prefixed the while paragraph with "can probably be put off until ...". > I think it's more likely for such BARs to be marked as read-only > (instead of denying access), in which case the checking here would > still be OK. Maybe, but (a) granting r/o access may still be beyond what an XSM policy intends and (b) might be a problem when reads have side effects. Jan
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index fd2cbf68bc62..c0ca57e05e98 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..12ae37deac83 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,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); + 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
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. Fixes: 9c244fdef7e7 ('vpci: add header handlers') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- 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 | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 7 deletions(-)