Re: [PATCH v4] xen/arm: validate non-common page-table ranges

[Gabriel Quintáns Souto <gabi.qs.mail@xxxxxxxxx>]

> There's no need to check for TEMPORARY area. It's only ever used in boot
> pgtables, not in runtime tables. Therefore what we need is just the following:
> virt < DOMHEAP_VIRT_START && size <= DOMHEAP_VIRT_START - virt
> without any helper.
>
> BTW. This patch breaks Arm32 MPU build. If at all, the helper should be placed
> under mmu/.

Ok, thanks for the review. I was not really sure whether TEMPORARY_AREA was
needed or not, so thanks for making that clear. One thing, though:
`virt < DOMHEAP_VIRT_START && size <= DOMHEAP_VIRT_START - virt`

To do that validation in pt.c, I would need to use `#ifdef CONFIG_ARM_32`
instead of `IS_ENABLED()` (which would let DCE drop it on arm64), otherwise it
doesn't build on arm64. That's why I made the xen_pt_range_is_common helper. How
should I approach this then? I'll drop `ranges_overlap()` either way, but
should I keep `xen_pt_range_is_common()` and move it under `mmu/`, or just drop
all the helpers and use `#ifdef CONFIG_ARM_32` directly in `pt.c`? I assume a
single `#ifdef` in `pt.c` is fine, so I'd go with the second option, as you
suggested. Let me know what you think.

>> +#include <asm/mmu/layout.h>
> Is this needed?

I think I added it while trying to get the helper to build, but I think
it's already pulled in through asm/config.h, so I'll likely drop it in v5.

Best regards,
Gabriel

On Thu Nov 12, 2026 at 3:26 PM CET, Michal Orzel wrote:
>
>
> On 10-Nov-26 15:52, Gabriel Quintáns Souto wrote:
>> On arm32, page-tables are different on each CPUs, although they share some
>> common mappings.
> Please say that the non-common area is the per-CPU domheap mapping.
>
>> 
>> The xen_pt_update function assumed that only said mappings would be
>> modified, but did not check this assumption.
>> 
>> Add a range check to reject modifications to non-common mappings.
>> 
>> Signed-off-by: Gabriel Quintáns Souto <gabi.qs.mail@xxxxxxxxx>
>> ---
>> v4:
>>  - Rebased onto staging (v3 was diffed against v2).
>> 
>> v3:
>>  - Dropped accidentally tracked patch file from commit.
>> 
>> v2:
>> - Inverted polarity of xen_pt_range_is_non_common() as suggested.
>> - Moved xen_pt_range_is_common() implementation to arch-specific headers
>> (arm32 vs arm64) instead of keeping #ifdef CONFIG_ARM_32 in pt.c (tried
>> IS_ENABLED() for DCE, but ARM64 lacks definitions for
>> DOMHEAP_VIRT_START/TEMPORARY_AREA_ADDR, breaking the build).
>> - Added ASSERT_UNREACHABLE() on failure path in xen_pt_update().
>> ---
>>  xen/arch/arm/include/asm/arm32/mm.h | 24 ++++++++++++++++++++++++
>>  xen/arch/arm/include/asm/arm64/mm.h | 10 ++++++++++
>>  xen/arch/arm/mmu/pt.c               | 14 +++++++-------
>>  3 files changed, 41 insertions(+), 7 deletions(-)
>> 
>> diff --git a/xen/arch/arm/include/asm/arm32/mm.h 
>> b/xen/arch/arm/include/asm/arm32/mm.h
>> index 856f2db..c7d09d3 100644
>> --- a/xen/arch/arm/include/asm/arm32/mm.h
>> +++ b/xen/arch/arm/include/asm/arm32/mm.h
>> @@ -16,6 +16,30 @@ static inline bool arch_mfns_in_directmap(unsigned long 
>> mfn, unsigned long nr)
>>      return false;
>>  }
>>  
>> +static inline bool ranges_overlap(unsigned long start1, unsigned long size1,
>> +                           unsigned long start2, unsigned long size2)
>> +{
>> +    if ( start1 < start2 )
>> +        return start2 - start1 < size1;
>> +
>> +    return start1 - start2 < size2;
>> +}
>> +
>> +/*
>> + * On ARM32, page-tables differ between CPUs, so not all mappings are 
>> common.
>> + */
>> +static inline bool xen_pt_range_is_common(unsigned long virt,
>> +                                       unsigned long size)
>> +{
>> +    unsigned long temporary_start = TEMPORARY_AREA_ADDR(0);
> There's no need to check for TEMPORARY area. It's only ever used in boot
> pgtables, not in runtime tables. Therefore what we need is just the following:
> virt < DOMHEAP_VIRT_START && size <= DOMHEAP_VIRT_START - virt
> without any helper.
>
> BTW. This patch breaks Arm32 MPU build. If at all, the helper should be placed
> under mmu/.
>
>
>> +    unsigned long temporary_size = XEN_PT_LEVEL_SIZE(1);
>> +
>> +    return !ranges_overlap(virt, size,
>> +                          DOMHEAP_VIRT_START, DOMHEAP_VIRT_SIZE) &&
>> +           !ranges_overlap(virt, size,
>> +                          temporary_start, temporary_size);
>> +}
>> +
>>  bool init_domheap_mappings(unsigned int cpu);
>>  
>>  static inline void arch_setup_page_tables(void)
>> diff --git a/xen/arch/arm/include/asm/arm64/mm.h 
>> b/xen/arch/arm/include/asm/arm64/mm.h
>> index b4f7545..a3cb56d 100644
>> --- a/xen/arch/arm/include/asm/arm64/mm.h
>> +++ b/xen/arch/arm/include/asm/arm64/mm.h
>> @@ -12,6 +12,16 @@ static inline bool arch_mfns_in_directmap(unsigned long 
>> mfn, unsigned long nr)
>>      return true;
>>  }
>>  
>> +/*
>> + * On ARM64, the Xen page-tables are shared by all the CPUs,
>> + * any range is therefore common.
>> + */
>> +static inline bool xen_pt_range_is_common(unsigned long virt,
>> +                                          unsigned long size)
>> +{
>> +    return true;
>> +}
>> +
>>  void arch_setup_page_tables(void);
>>  
>>  void update_boot_mapping(bool enable);
>> diff --git a/xen/arch/arm/mmu/pt.c b/xen/arch/arm/mmu/pt.c
>> index 81fe121..42fa79a 100644
>> --- a/xen/arch/arm/mmu/pt.c
>> +++ b/xen/arch/arm/mmu/pt.c
>> @@ -13,6 +13,7 @@
>>  
>>  #include <asm/current.h>
>>  #include <asm/fixmap.h>
>> +#include <asm/mmu/layout.h>
> Is this needed?
>
> ~Michal