[PATCH] x86/setup: correct off-by-1 in module mapping

[PATCH] x86/setup: correct off-by-1 in module mapping

[Jan Beulich]

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));
         map_pages_to_xen((unsigned long)maddr_to_virt(s), maddr_to_mfn(s),
                          PFN_UP(l), PAGE_HYPERVISOR);
     }

Re: [PATCH] x86/setup: correct off-by-1 in module mapping

[Roger Pau Monné]

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.

Re: [PATCH] x86/setup: correct off-by-1 in module mapping

[Andrew Cooper]

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

Re: [PATCH] x86/setup: correct off-by-1 in module mapping

[Jan Beulich]

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

Re: [PATCH] x86/setup: correct off-by-1 in module mapping

[Roger Pau Monné]

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.