| Message ID | ad60ded1-5feb-4fd9-8cf8-6dee2e153d11@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Arm: correct FIX_LAST | expand |
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
--- 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)
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.