Hi, Jan!
On 10.12.21 11:40, Jan Beulich wrote:
> 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
> ---
> v2: Add comment to BUILD_BUG_ON(). Avoid transiently #define-ing _gfn()
> (by way of new prereq patch).
>
> --- 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));
Technically this is ok, but reads odd now: the function maps a single
page, but its name has "region" in it (which might also be ok, e.g.
for a region of a single page).
I think it is worth either implementing full mfn range check inside
map_dev_mmio_region or renaming it to something else:
with mfn check map_dev_mmio_region will indeed be able to map
a region consisting of multiple pages and perform required validation.
Thank you,
Oleksandr
> return rc;
>
> default:
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1357,19 +1357,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
> @@ -4150,7 +4150,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
> @@ -832,6 +832,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;
>
> @@ -842,6 +845,18 @@ 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 )
> +{
> +/*
> + * Make sure INVALID_GFN is the highest representable value, i.e.
> + * guaranteeing that it won't fall in the middle of the
> + * [xatp->gpfn, xatp->gpfn + xatp->size) range checked above.
> + */
> +BUILD_BUG_ON(INVALID_GFN_RAW + 1);
> +return -EOVERFLOW;
> +}
> +
> xatp->idx += start;
> xatp->gpfn += start;
> xatp->size -= start;
> @@ -962,6 +977,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 p2m_insert_mapping(struct domain *d, gfn_t start_gfn, unsigned long nr,
>
>
