While reviewing a RISC-V patch cloning the Arm code, I noticed an
off-by-1 here: FIX_PMAP_{BEGIN,END} being an inclusive range and
FIX_LAST being the same as FIX_PMAP_END, FIXADDR_TOP cannot derive from
FIX_LAST alone, or else the BUG_ON() in virt_to_fix() would trigger if
FIX_PMAP_END ended up being used.

While touching this area also add a check for fixmap and boot FDT area
to not only not overlap, but to have at least one (unmapped) page in
between.

Fixes: 4f17357b52f6 ("xen/arm: add Persistent Map (PMAP) infrastructure")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Adjust FIXADDR_TOP instead. Add BUILD_BUG_ON() as suggested by
    Michal.

--- a/xen/arch/arm/include/asm/fixmap.h
+++ b/xen/arch/arm/include/asm/fixmap.h
@@ -18,7 +18,7 @@
 #define FIX_LAST FIX_PMAP_END
 
 #define FIXADDR_START FIXMAP_ADDR(0)
-#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)
+#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST + 1)
 
 #ifndef __ASSEMBLY__
 
--- a/xen/arch/arm/mmu/setup.c
+++ b/xen/arch/arm/mmu/setup.c
@@ -128,6 +128,12 @@ static void __init __maybe_unused build_
 
 #undef CHECK_SAME_SLOT
 #undef CHECK_DIFFERENT_SLOT
+
+    /*
+     * Fixmaps must not overlap with boot FDT mapping area. Make sure there's
+     * at least one guard page in between.
+     */
+    BUILD_BUG_ON(FIXADDR_TOP >= BOOT_FDT_VIRT_START);
 }
 
 lpae_t __init pte_of_xenaddr(vaddr_t va)

On 13/08/2024 13:49, Jan Beulich wrote:
> 
> 
> While reviewing a RISC-V patch cloning the Arm code, I noticed an
> off-by-1 here: FIX_PMAP_{BEGIN,END} being an inclusive range and
> FIX_LAST being the same as FIX_PMAP_END, FIXADDR_TOP cannot derive from
> FIX_LAST alone, or else the BUG_ON() in virt_to_fix() would trigger if
> FIX_PMAP_END ended up being used.
> 
> While touching this area also add a check for fixmap and boot FDT area
> to not only not overlap, but to have at least one (unmapped) page in
> between.
> 
> Fixes: 4f17357b52f6 ("xen/arm: add Persistent Map (PMAP) infrastructure")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Hi,

On 13/08/2024 12:57, Michal Orzel wrote:
> 
> 
> On 13/08/2024 13:49, Jan Beulich wrote:
>>
>>
>> While reviewing a RISC-V patch cloning the Arm code, I noticed an
>> off-by-1 here: FIX_PMAP_{BEGIN,END} being an inclusive range and
>> FIX_LAST being the same as FIX_PMAP_END, FIXADDR_TOP cannot derive from
>> FIX_LAST alone, or else the BUG_ON() in virt_to_fix() would trigger if
>> FIX_PMAP_END ended up being used.
>>
>> While touching this area also add a check for fixmap and boot FDT area
>> to not only not overlap, but to have at least one (unmapped) page in
>> between.
>>
>> Fixes: 4f17357b52f6 ("xen/arm: add Persistent Map (PMAP) infrastructure")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Committed.

Cheers,

-- 
Julien Grall