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, FIX_LAST
cannot be the same as FIX_PMAP_END, or else the BUG_ON() in
virt_to_fix() would trigger if FIX_PMAP_END ended up being used.

Fixes: 4f17357b52f6 ("xen/arm: add Persistent Map (PMAP) infrastructure")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Alternatively the definition of FIXADDR_TOP could be changed, if "last"
should retain its strict meaning. Possibly a guard page also wants
having at the end of the fixmap range, which could be effected by
changing both #define-s at the same time.

--- a/xen/arch/arm/include/asm/fixmap.h
+++ b/xen/arch/arm/include/asm/fixmap.h
@@ -15,7 +15,7 @@
 #define FIX_PMAP_BEGIN (FIX_ACPI_END + 1) /* Start of PMAP */
 #define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */
 
-#define FIX_LAST FIX_PMAP_END
+#define FIX_LAST (FIX_PMAP_END + 1)
 
 #define FIXADDR_START FIXMAP_ADDR(0)
 #define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)

On 13/08/2024 10:36, 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, FIX_LAST
> cannot be the same as FIX_PMAP_END, or else the BUG_ON() in
> virt_to_fix() would trigger if FIX_PMAP_END ended up being used.
> 
> Fixes: 4f17357b52f6 ("xen/arm: add Persistent Map (PMAP) infrastructure")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Alternatively the definition of FIXADDR_TOP could be changed, if "last"
> should retain its strict meaning. Possibly a guard page also wants
> having at the end of the fixmap range, which could be effected by
> changing both #define-s at the same time.
I understand FIX_LAST as the last slot used, so in this regard it should retain its definition.
That said, I realized that we don't even check that the highest slot is still within the max number
of fixmap slots (today we have 512 slots). IMO we should gain a BUILD_BUG_ON to catch it.

With:
#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST + 1)
and sth like:
BUILD_BUG_ON(FIXADDR_TOP >= BOOT_FDT_VIRT_START);
possibly in build_assertions() in setup.c, we would:
 - fix off-by-1 issue
 - catch out-of-slot issue
 - gain a guard page

~Michal