| Message ID | 110f0966-0b89-48d6-b1b3-abae93802c19@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] x86/PVH: account for module command line length | expand |
On 2025-03-24 08:03, Jan Beulich wrote: > As per observation in practice, initrd->cmdline_pa is not normally zero. > Hence so far we always appended at least one byte. That alone may > already render insufficient the "allocation" made by find_memory(). > Things would be worse when there's actually a (perhaps long) command > line. > > Skip setup when the command line is empty. Amend the "allocation" size > by padding and actual size of module command line. Along these lines > also skip initrd setup when the initrd is zero size. > > Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
On 24/03/2025 12:03 pm, Jan Beulich wrote: > As per observation in practice, initrd->cmdline_pa is not normally zero. > Hence so far we always appended at least one byte. That alone may > already render insufficient the "allocation" made by find_memory(). > Things would be worse when there's actually a (perhaps long) command > line. > > Skip setup when the command line is empty. Amend the "allocation" size > by padding and actual size of module command line. Along these lines > also skip initrd setup when the initrd is zero size. > > Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v3: Permit empty initrd with non-empty module command line again. > v2: Use elf_round_up(). Introduce initrd_cmdline local. Re-base. I wasn't making a request to support an empty module with a non-empty cmdline. That's a nonsense configuration; it's almost certainly an error elsewhere, rather than an intended configuration. I was simply requesting that the implications are considered, and discussed, rather than silently dropping. We ought to exclude such cases centrally, rather than locally in the PVH path. > > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -652,9 +652,10 @@ static int __init pvh_load_kernel( > unsigned long image_len = image->size; > unsigned long initrd_len = initrd ? initrd->size : 0; > const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL; > + const char *initrd_cmdline = NULL; > struct elf_binary elf; > struct elf_dom_parms parms; > - size_t extra_space; > + size_t extra_space = 0; > paddr_t last_addr; > struct hvm_start_info start_info = { 0 }; > struct hvm_modlist_entry mod = { 0 }; > @@ -712,10 +713,26 @@ static int __init pvh_load_kernel( > * split into smaller allocations, done as a single region in order to > * simplify it. > */ > - extra_space = sizeof(start_info); > - > if ( initrd ) > - extra_space += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); > + { > + extra_space = elf_round_up(&elf, initrd_len); I'm going to insist on not doing this. For the sake of anyone (else) trying to follow this logic. You're trading increased cognitive complexity of the logic to avoid using one local variable. ~Andrew
On 24.03.2025 19:24, Andrew Cooper wrote: > On 24/03/2025 12:03 pm, Jan Beulich wrote: >> As per observation in practice, initrd->cmdline_pa is not normally zero. >> Hence so far we always appended at least one byte. That alone may >> already render insufficient the "allocation" made by find_memory(). >> Things would be worse when there's actually a (perhaps long) command >> line. >> >> Skip setup when the command line is empty. Amend the "allocation" size >> by padding and actual size of module command line. Along these lines >> also skip initrd setup when the initrd is zero size. >> >> Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> v3: Permit empty initrd with non-empty module command line again. >> v2: Use elf_round_up(). Introduce initrd_cmdline local. Re-base. > > I wasn't making a request to support an empty module with a non-empty > cmdline. > > That's a nonsense configuration; it's almost certainly an error > elsewhere, rather than an intended configuration. Who are we to judge whether some configuration is nonsense? >> --- a/xen/arch/x86/hvm/dom0_build.c >> +++ b/xen/arch/x86/hvm/dom0_build.c >> @@ -652,9 +652,10 @@ static int __init pvh_load_kernel( >> unsigned long image_len = image->size; >> unsigned long initrd_len = initrd ? initrd->size : 0; >> const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL; >> + const char *initrd_cmdline = NULL; >> struct elf_binary elf; >> struct elf_dom_parms parms; >> - size_t extra_space; >> + size_t extra_space = 0; >> paddr_t last_addr; >> struct hvm_start_info start_info = { 0 }; >> struct hvm_modlist_entry mod = { 0 }; >> @@ -712,10 +713,26 @@ static int __init pvh_load_kernel( >> * split into smaller allocations, done as a single region in order to >> * simplify it. >> */ >> - extra_space = sizeof(start_info); >> - >> if ( initrd ) >> - extra_space += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); >> + { >> + extra_space = elf_round_up(&elf, initrd_len); > > I'm going to insist on not doing this. For the sake of anyone (else) > trying to follow this logic. > > You're trading increased cognitive complexity of the logic to avoid > using one local variable. Well, okay, I'll make a change here then. Your earlier reply didn't sound like "I'm going to insist", though. Instead I thought I could assume that my reply there having been left un-responded to would mean you're okay with the justification I gave. Jan
--- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -652,9 +652,10 @@ static int __init pvh_load_kernel( unsigned long image_len = image->size; unsigned long initrd_len = initrd ? initrd->size : 0; const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL; + const char *initrd_cmdline = NULL; struct elf_binary elf; struct elf_dom_parms parms; - size_t extra_space; + size_t extra_space = 0; paddr_t last_addr; struct hvm_start_info start_info = { 0 }; struct hvm_modlist_entry mod = { 0 }; @@ -712,10 +713,26 @@ static int __init pvh_load_kernel( * split into smaller allocations, done as a single region in order to * simplify it. */ - extra_space = sizeof(start_info); - if ( initrd ) - extra_space += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); + { + extra_space = elf_round_up(&elf, initrd_len); + + if ( initrd->cmdline_pa ) + { + initrd_cmdline = __va(initrd->cmdline_pa); + if ( !*initrd_cmdline ) + initrd_cmdline = NULL; + } + if ( initrd_cmdline ) + extra_space += strlen(initrd_cmdline) + 1; + + if ( extra_space ) + extra_space = ROUNDUP(extra_space, PAGE_SIZE) + sizeof(mod); + else + initrd = NULL; + } + + extra_space += sizeof(start_info); if ( cmdline ) extra_space += elf_round_up(&elf, strlen(cmdline) + 1); @@ -740,12 +757,11 @@ static int __init pvh_load_kernel( mod.paddr = last_addr; mod.size = initrd_len; last_addr += elf_round_up(&elf, initrd_len); - if ( initrd->cmdline_pa ) + if ( initrd_cmdline ) { - char *str = __va(initrd->cmdline_pa); - size_t len = strlen(str) + 1; + size_t len = strlen(initrd_cmdline) + 1; - rc = hvm_copy_to_guest_phys(last_addr, str, len, v); + rc = hvm_copy_to_guest_phys(last_addr, initrd_cmdline, len, v); if ( rc ) { printk("Unable to copy module command line\n");
As per observation in practice, initrd->cmdline_pa is not normally zero. Hence so far we always appended at least one byte. That alone may already render insufficient the "allocation" made by find_memory(). Things would be worse when there's actually a (perhaps long) command line. Skip setup when the command line is empty. Amend the "allocation" size by padding and actual size of module command line. Along these lines also skip initrd setup when the initrd is zero size. Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v3: Permit empty initrd with non-empty module command line again. v2: Use elf_round_up(). Introduce initrd_cmdline local. Re-base.