| Message ID | f346b510-dbd1-431a-ad35-3f1b8fe76c58@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/setup: correct off-by-1 in module mapping | expand |
On Wed, Mar 19, 2025 at 12:21:53PM +0100, Jan Beulich wrote: > If a module's length is an exact multiple of PAGE_SIZE, the 2nd argument > passed to set_pdx_range() would be one larger than intended. Use > PFN_{UP,DOWN}() there instead. > > Fixes: cd7cc5320bb2 ("x86/boot: add start and size fields to struct boot_module") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -1730,7 +1730,7 @@ void asmlinkage __init noreturn __start_ > { > unsigned long s = bi->mods[i].start, l = bi->mods[i].size; > > - set_pdx_range(paddr_to_pfn(s), paddr_to_pfn(s + l) + 1); > + set_pdx_range(PFN_DOWN(s), PFN_UP(s + l)); Is the set_pdx_range() provided smfn, emfn range inclusive? > map_pages_to_xen((unsigned long)maddr_to_virt(s), maddr_to_mfn(s), > PFN_UP(l), PAGE_HYPERVISOR); Isn't this line also bogus, as there's no guarantee that the start address is aligned to PAGE_SIZE? And hence the length should take into account a possible page offset of s? Thanks, Roger.
On 19/03/2025 12:15 pm, Roger Pau Monné wrote: > On Wed, Mar 19, 2025 at 12:21:53PM +0100, Jan Beulich wrote: >> If a module's length is an exact multiple of PAGE_SIZE, the 2nd argument >> passed to set_pdx_range() would be one larger than intended. Use >> PFN_{UP,DOWN}() there instead. >> >> Fixes: cd7cc5320bb2 ("x86/boot: add start and size fields to struct boot_module") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1730,7 +1730,7 @@ void asmlinkage __init noreturn __start_ >> { >> unsigned long s = bi->mods[i].start, l = bi->mods[i].size; >> >> - set_pdx_range(paddr_to_pfn(s), paddr_to_pfn(s + l) + 1); >> + set_pdx_range(PFN_DOWN(s), PFN_UP(s + l)); > Is the set_pdx_range() provided smfn, emfn range inclusive? > >> map_pages_to_xen((unsigned long)maddr_to_virt(s), maddr_to_mfn(s), >> PFN_UP(l), PAGE_HYPERVISOR); > Isn't this line also bogus, as there's no guarantee that the start > address is aligned to PAGE_SIZE? And hence the length should take > into account a possible page offset of s? All modules are page aligned, because we require this of the bootloader. Prior to cd7cc5320bb2, Xen strictly depended on it, because we shifted mod_start by 12 to operate on it. cd7cc5320bb2 retained the alignment check, panic("Bootloader didn't honor module alignment request\n");, but operates in terms of paddr, not frames. ~Andrew
On 19.03.2025 13:15, Roger Pau Monné wrote: > On Wed, Mar 19, 2025 at 12:21:53PM +0100, Jan Beulich wrote: >> If a module's length is an exact multiple of PAGE_SIZE, the 2nd argument >> passed to set_pdx_range() would be one larger than intended. Use >> PFN_{UP,DOWN}() there instead. >> >> Fixes: cd7cc5320bb2 ("x86/boot: add start and size fields to struct boot_module") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -1730,7 +1730,7 @@ void asmlinkage __init noreturn __start_ >> { >> unsigned long s = bi->mods[i].start, l = bi->mods[i].size; >> >> - set_pdx_range(paddr_to_pfn(s), paddr_to_pfn(s + l) + 1); >> + set_pdx_range(PFN_DOWN(s), PFN_UP(s + l)); > > Is the set_pdx_range() provided smfn, emfn range inclusive? No, it's exclusive at the end. See all other uses in this file. >> map_pages_to_xen((unsigned long)maddr_to_virt(s), maddr_to_mfn(s), >> PFN_UP(l), PAGE_HYPERVISOR); > > Isn't this line also bogus, as there's no guarantee that the start > address is aligned to PAGE_SIZE? And hence the length should take > into account a possible page offset of s? See Andrew's reply. Jan
On Wed, Mar 19, 2025 at 12:21:53PM +0100, Jan Beulich wrote: > If a module's length is an exact multiple of PAGE_SIZE, the 2nd argument > passed to set_pdx_range() would be one larger than intended. Use > PFN_{UP,DOWN}() there instead. > > Fixes: cd7cc5320bb2 ("x86/boot: add start and size fields to struct boot_module") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
--- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1730,7 +1730,7 @@ void asmlinkage __init noreturn __start_ { unsigned long s = bi->mods[i].start, l = bi->mods[i].size; - set_pdx_range(paddr_to_pfn(s), paddr_to_pfn(s + l) + 1); + set_pdx_range(PFN_DOWN(s), PFN_UP(s + l)); map_pages_to_xen((unsigned long)maddr_to_virt(s), maddr_to_mfn(s), PFN_UP(l), PAGE_HYPERVISOR); }
If a module's length is an exact multiple of PAGE_SIZE, the 2nd argument passed to set_pdx_range() would be one larger than intended. Use PFN_{UP,DOWN}() there instead. Fixes: cd7cc5320bb2 ("x86/boot: add start and size fields to struct boot_module") Signed-off-by: Jan Beulich <jbeulich@suse.com>