Re: [PATCH 1/3] xen/arm: Fix off-by-one in iomem_deny_access() calls

["Halder, Ayan Kumar" <ayankuma@xxxxxxx>]

Hi Michal,

Apologies if my review is weird, I have been looking into too much of 
safety stuff.
On 09/04/2026 12:39, Michal Orzel wrote:
> iomem_deny_access() wraps rangeset_remove_range() which takes inclusive
> endpoints.  All call sites in the GIC and ACPI code pass 'mfn + nr' (or
> 'mfn + 1' for single-page regions) as the end parameter, which causes
> one extra page beyond each region to be denied.
>
> For single-page regions, use 'mfn' as the end (denying exactly one page).
> For all multi-page regions, use 'mfn + nr - 1'.
Just reading this and the change below, it seems that the issue was 
caught while doing some boundary value analysis. In this specific case, 
it seems the boundary values were set incorrectly.
Can you explain a bit more (the boundary/edge cases) in the commit 
message and give some reference to test (can be even a different repo or 
something) on how you caught this and verified it to be correct ?
We can keep this test somewhere (and tag it to the commit) even if such 
tests does not make sense to be upstreamed.
- Ayan

> This matches the correct pattern used elsewhere, e.g. in device.c.
>
> Fixes: 8300b3377e ("arm/gic: Add a new callback to deny Dom0 access to GIC 
> regions")
> Fixes: 66158be465 ("ARM: ITS: Deny hardware domain access to ITS")
> Fixes: 97e9875646 ("arm/acpi: Permit MMIO access of Xen unused devices for 
> Dom0")
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>   xen/arch/arm/acpi/domain_build.c | 2 +-
>   xen/arch/arm/gic-v2.c            | 8 ++++----
>   xen/arch/arm/gic-v3-its.c        | 2 +-
>   xen/arch/arm/gic-v3.c            | 8 ++++----
>   4 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
> index 5a117001ef11..249d899c3337 100644
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -48,7 +48,7 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>       {
>           mfn = spcr->serial_port.address >> PAGE_SHIFT;
>           /* Deny MMIO access for UART */
> -        rc = iomem_deny_access(d, mfn, mfn + 1);
> +        rc = iomem_deny_access(d, mfn, mfn);
>           if ( rc )
>               return rc;
>       }
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index b23e72a3d05d..014f9559673b 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -1079,23 +1079,23 @@ static int gicv2_iomem_deny_access(struct domain *d)
>       unsigned long mfn, nr;
>   
>       mfn = dbase >> PAGE_SHIFT;
> -    rc = iomem_deny_access(d, mfn, mfn + 1);
> +    rc = iomem_deny_access(d, mfn, mfn);
>       if ( rc )
>           return rc;
>   
>       mfn = hbase >> PAGE_SHIFT;
> -    rc = iomem_deny_access(d, mfn, mfn + 1);
> +    rc = iomem_deny_access(d, mfn, mfn);
>       if ( rc )
>           return rc;
>   
>       mfn = cbase >> PAGE_SHIFT;
>       nr = DIV_ROUND_UP(csize, PAGE_SIZE);
> -    rc = iomem_deny_access(d, mfn, mfn + nr);
> +    rc = iomem_deny_access(d, mfn, mfn + nr - 1);
>       if ( rc )
>           return rc;
>   
>       mfn = vbase >> PAGE_SHIFT;
> -    return iomem_deny_access(d, mfn, mfn + nr);
> +    return iomem_deny_access(d, mfn, mfn + nr - 1);
>   }
>   
>   #ifdef CONFIG_ACPI
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 9ba068c46fcb..e38aa8711744 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -1009,7 +1009,7 @@ int gicv3_its_deny_access(struct domain *d)
>       {
>           mfn = paddr_to_pfn(its_data->addr);
>           nr = PFN_UP(its_data->size);
> -        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        rc = iomem_deny_access(d, mfn, mfn + nr - 1);
>           if ( rc )
>           {
>               printk("iomem_deny_access failed for %lx:%lx \r\n", mfn, nr);
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index bc07f97c16ab..b3e104ea4ad0 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1602,7 +1602,7 @@ static int gicv3_iomem_deny_access(struct domain *d)
>   
>       mfn = dbase >> PAGE_SHIFT;
>       nr = PFN_UP(SZ_64K);
> -    rc = iomem_deny_access(d, mfn, mfn + nr);
> +    rc = iomem_deny_access(d, mfn, mfn + nr - 1);
>       if ( rc )
>           return rc;
>   
> @@ -1614,7 +1614,7 @@ static int gicv3_iomem_deny_access(struct domain *d)
>       {
>           mfn = gicv3.rdist_regions[i].base >> PAGE_SHIFT;
>           nr = PFN_UP(gicv3.rdist_regions[i].size);
> -        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        rc = iomem_deny_access(d, mfn, mfn + nr - 1);
>           if ( rc )
>               return rc;
>       }
> @@ -1623,7 +1623,7 @@ static int gicv3_iomem_deny_access(struct domain *d)
>       {
>           mfn = cbase >> PAGE_SHIFT;
>           nr = PFN_UP(csize);
> -        rc = iomem_deny_access(d, mfn, mfn + nr);
> +        rc = iomem_deny_access(d, mfn, mfn + nr - 1);
>           if ( rc )
>               return rc;
>       }
> @@ -1632,7 +1632,7 @@ static int gicv3_iomem_deny_access(struct domain *d)
>       {
>           mfn = vbase >> PAGE_SHIFT;
>           nr = PFN_UP(csize);
> -        return iomem_deny_access(d, mfn, mfn + nr);
> +        return iomem_deny_access(d, mfn, mfn + nr - 1);
>       }
>   
>       return 0;