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'.
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@amd.com>
---
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;
--
2.43.0Hi Michal,
> On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> 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'.
>
> 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@amd.com>
> ---
>
This looks ok to me.
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Cheers,
Luca
On Thu, 9 Apr 2026, Luca Fancellu wrote:
> Hi Michal,
>
> > On 9 Apr 2026, at 12:39, Michal Orzel <michal.orzel@amd.com> 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'.
> >
> > 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@amd.com>
> > ---
> >
>
> This looks ok to me.
>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@amd.com>
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@amd.com>
> ---
> 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;
On 09/04/2026 13:49, Halder, Ayan Kumar wrote:
> Hi Michal,
>
> Apologies if my review is weird, I have been looking into too much of
> safety stuff.
Yes, I know :)
>
> 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.
As much as it looks like an issue found during BVA, the truth is I found it by
accident when debugging some issue where rangesets were involved. As always,
whenever I see a function with parameters {start, end}, I need to dive deeper to
verify whether end is inclusive or not. I also check the call-sites to validate
my observations and that's how I found these bugs.
~Michal
© 2016 - 2026 Red Hat, Inc.