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@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,

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@suse.com>
> ---
> 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

On 14.10.2021 13:29, Julien Grall wrote:
> 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@suse.com>
>> ---
>> 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.

Odd, but I clearly don't understand all the aspects here.

>> 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.

My point was rather about this potentially being dead code in non-ACPI
builds (i.e. in particular uniformly on 32-bit).

>> @@ -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.

No - _gfn() is an inline function as per typesafe.h. (Or else it
wouldn't be just "some" compiler, but gcc at least would have
complained to me.)

> However...
> 
>> +        BUILD_BUG_ON(INVALID_GFN + 1);
> 
> ... I might be missing something... but why can't use gfn_x(INVALID_GFN) 
> + 1 here?

Because gfn_x() also is an inline function, and that's not suitable
for a compile-time constant expression.

> In fact, I am not entirely sure what's the purpose of this 
> BUILD_BUG_ON(). Could you give more details?

The expression in the surrounding if() relies on INVALID_GFN being the
largest representable value, i.e. this ensures that INVALID_GFN doesn't
sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size).

Jan

Hi,

On 14/10/2021 15:10, Jan Beulich wrote:
> On 14.10.2021 13:29, Julien Grall wrote:
>> 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@suse.com>
>>> ---
>>> 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.
> 
> Odd, but I clearly don't understand all the aspects here.
> 
>>> 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.
> 
> My point was rather about this potentially being dead code in non-ACPI
> builds (i.e. in particular uniformly on 32-bit).

The hypercall is already wired and a dom0 OS can use it today even on 
non-ACPI. Whether a dom0 OS will use it is a different question. I know 
that Linux will limit it to ACPI. It is likely not used by other OS, but 
I can't guarantee it.

In this case, the hypercall is only a few lines and already restricted 
to dom0 only (see xapt_permission_check()). So to me, the #ifdef here is 
not worth it.

>>> @@ -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.
> 
> No - _gfn() is an inline function as per typesafe.h. (Or else it
> wouldn't be just "some" compiler, but gcc at least would have
> complained to me.)

Ah. somehow I thought it was a macro. But looking at the implementation, 
it makes sense to be an inline funciton.

Sorry for the noise.

> 
>> However...
>>
>>> +        BUILD_BUG_ON(INVALID_GFN + 1);
>>
>> ... I might be missing something... but why can't use gfn_x(INVALID_GFN)
>> + 1 here?
> 
> Because gfn_x() also is an inline function, and that's not suitable
> for a compile-time constant expression.

Right. How about introduce INVALID_GFN_RAW in mm-frame.h? This could 
also be used to replace the open-code value in INVALID_GFN and 
INVALID_GFN_INITIALIZER?

> 
>> In fact, I am not entirely sure what's the purpose of this
>> BUILD_BUG_ON(). Could you give more details?
> 
> The expression in the surrounding if() relies on INVALID_GFN being the
> largest representable value, i.e. this ensures that INVALID_GFN doesn't
> sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size).

Thanks the explanation. Can you add the rationale in a comment on top of 
BUILD_BUG_ON()?

Cheers,

-- 
Julien Grall

On 15.10.2021 11:26, Julien Grall wrote:
> On 14/10/2021 15:10, Jan Beulich wrote:
>> On 14.10.2021 13:29, Julien Grall wrote:
>>> 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@suse.com>
>>>> ---
>>>> 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.
>>
>> Odd, but I clearly don't understand all the aspects here.
>>
>>>> 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.
>>
>> My point was rather about this potentially being dead code in non-ACPI
>> builds (i.e. in particular uniformly on 32-bit).
> 
> The hypercall is already wired and a dom0 OS can use it today even on 
> non-ACPI. Whether a dom0 OS will use it is a different question. I know 
> that Linux will limit it to ACPI. It is likely not used by other OS, but 
> I can't guarantee it.
> 
> In this case, the hypercall is only a few lines and already restricted 
> to dom0 only (see xapt_permission_check()). So to me, the #ifdef here is 
> not worth it.

Well, okay then - I've removed that remark.

>>>> @@ -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.
>>
>> No - _gfn() is an inline function as per typesafe.h. (Or else it
>> wouldn't be just "some" compiler, but gcc at least would have
>> complained to me.)
> 
> Ah. somehow I thought it was a macro. But looking at the implementation, 
> it makes sense to be an inline funciton.
> 
> Sorry for the noise.
> 
>>
>>> However...
>>>
>>>> +        BUILD_BUG_ON(INVALID_GFN + 1);
>>>
>>> ... I might be missing something... but why can't use gfn_x(INVALID_GFN)
>>> + 1 here?
>>
>> Because gfn_x() also is an inline function, and that's not suitable
>> for a compile-time constant expression.
> 
> Right. How about introduce INVALID_GFN_RAW in mm-frame.h? This could 
> also be used to replace the open-code value in INVALID_GFN and 
> INVALID_GFN_INITIALIZER?

Can do, but that'll be a prereq patch then also taking care of INVALID_MFN.

>>> In fact, I am not entirely sure what's the purpose of this
>>> BUILD_BUG_ON(). Could you give more details?
>>
>> The expression in the surrounding if() relies on INVALID_GFN being the
>> largest representable value, i.e. this ensures that INVALID_GFN doesn't
>> sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size).
> 
> Thanks the explanation. Can you add the rationale in a comment on top of 
> BUILD_BUG_ON()?

Sure, done.

Jan