Determining that behavior is correct (i.e. results in failure) for a
passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a
bit more obvious by checking input in generic code - both for singular
requests to not match the value and for range ones to not pass / wrap
through it.
For Arm similarly make more obvious that no wrapping of MFNs passed
for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
Drop the "nr" parameter of the function to avoid future callers
appearing which might not themselves check for wrapping. Otherwise
the respective ASSERT() in rangeset_contains_range() could trigger.
Signed-off-by: Jan Beulich <jbeul...@suse.com>
---
I find it odd that map_dev_mmio_region() returns success upon
iomem_access_permitted() indicating failure - is this really intended?
As per commit 102984bb1987 introducing it this also was added for ACPI
only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI
builds?
I'd be happy to take suggestions towards avoiding the need to #define
_gfn() around the BUILD_BUG_ON() being added. I couldn't think of
anything prettier.
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1479,7 +1479,7 @@ int xenmem_add_to_physmap_one(
break;
}
case XENMAPSPACE_dev_mmio:
- rc = map_dev_mmio_region(d, gfn, 1, _mfn(idx));
+ rc = map_dev_mmio_region(d, gfn, _mfn(idx));
return rc;
default:
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1360,19 +1360,18 @@ int unmap_mmio_regions(struct domain *d,
int map_dev_mmio_region(struct domain *d,
gfn_t gfn,
- unsigned long nr,
mfn_t mfn)
{
int res;
- if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) )
+ if ( !iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn)) )
return 0;
- res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
+ res = p2m_insert_mapping(d, gfn, 1, mfn, p2m_mmio_direct_c);
if ( res < 0 )
{
- printk(XENLOG_G_ERR "Unable to map MFNs [%#"PRI_mfn" - %#"PRI_mfn" in
Dom%d\n",
- mfn_x(mfn), mfn_x(mfn) + nr - 1, d->domain_id);
+ printk(XENLOG_G_ERR "Unable to map MFN %#"PRI_mfn" in %pd\n",
+ mfn_x(mfn), d);
return res;
}
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -4132,7 +4132,10 @@ int gnttab_map_frame(struct domain *d, u
bool status = false;
if ( gfn_eq(gfn, INVALID_GFN) )
+ {
+ ASSERT_UNREACHABLE();
return -EINVAL;
+ }
grant_write_lock(gt);
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -831,6 +831,9 @@ int xenmem_add_to_physmap(struct domain
return -EACCES;
}
+ if ( gfn_eq(_gfn(xatp->gpfn), INVALID_GFN) )
+ return -EINVAL;
+
if ( xatp->space == XENMAPSPACE_gmfn_foreign )
extra.foreign_domid = DOMID_INVALID;
@@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain
if ( xatp->size < start )
return -EILSEQ;
+ if ( xatp->gpfn + xatp->size < xatp->gpfn ||
+ xatp->idx + xatp->size < xatp->idx )
+ {
+#define _gfn(x) (x)
+ BUILD_BUG_ON(INVALID_GFN + 1);
+#undef _gfn
+ return -EOVERFLOW;
+ }
+
xatp->idx += start;
xatp->gpfn += start;
xatp->size -= start;
@@ -961,6 +973,9 @@ static int xenmem_add_to_physmap_batch(s
extent, 1)) )
return -EFAULT;
+ if ( gfn_eq(_gfn(gpfn), INVALID_GFN) )
+ return -EINVAL;
+
rc = xenmem_add_to_physmap_one(d, xatpb->space, extra,
idx, _gfn(gpfn));
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -297,7 +297,6 @@ int unmap_regions_p2mt(struct domain *d,
int map_dev_mmio_region(struct domain *d,
gfn_t gfn,
- unsigned long nr,
mfn_t mfn);
int guest_physmap_add_entry(struct domain *d,
