Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 13/09/2021 07:42, 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 <jbeulich@xxxxxxxx>
> ---
> I find it odd that map_dev_mmio_region() returns success upon
> iomem_access_permitted() indicating failure - is this really intended?
AFAIR yes. The hypercall is not used as "Map the region" but instead 
"Make sure the region is mapped if the IOMEM region is accessible".
It is necessary to return 0 because dom0 OS cannot distinguished between 
emulated and non-emulated. So we may report error when there is none.
> 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?
There is nothing specific to ACPI in the implementation. So I don't 
really see the reason to restrict to CONFIG_ACPI.
However, it is still possible to boot using DT when Xen is built with 
CONFIG_ACPI. So if the restriction was desirable, then I think it should 
be using !acpi_disabled.
> 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)
AFAICT, _gfn() will already be defined. So some compiler may complain 
because will be defined differently on debug build. However...
> +        BUILD_BUG_ON(INVALID_GFN + 1);
... I might be missing something... but why can't use gfn_x(INVALID_GFN) 
+ 1 here?
In fact, I am not entirely sure what's the purpose of this 
BUILD_BUG_ON(). Could you give more details?
> +#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,
Cheers,

--
Julien Grall