| Message ID | 730d8143-8cda-49da-a48a-3b82c2b77c9d@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/PVH: account for module command line length | expand |
On 19/03/2025 11:18 am, 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. > > Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -712,7 +712,15 @@ static int __init pvh_load_kernel( > * simplify it. > */ > last_addr = find_memory(d, &elf, sizeof(start_info) + > - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + > + (initrd ? ROUNDUP(ROUNDUP(initrd_len, > + elf_64bit(&elf) ? 8 : 4) + > + (initrd->cmdline_pa && > + strlen(__va(initrd-> > + cmdline_pa)) > + ? strlen(__va(initrd-> > + cmdline_pa)) + 1 > + : 0), > + PAGE_SIZE) + > sizeof(mod) > : 0) + > (cmdline ? ROUNDUP(strlen(cmdline) + 1, This piece of logic was already bad, but this is rather worse. One patch I made while doing the boot module work is: diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 6a4453103a9a..7292ddd07276 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -654,6 +654,7 @@ static int __init pvh_load_kernel( const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : NULL; struct elf_binary elf; struct elf_dom_parms parms; + size_t metadata_len; paddr_t last_addr; struct hvm_start_info start_info = { 0 }; struct hvm_modlist_entry mod = { 0 }; @@ -711,13 +712,16 @@ static int __init pvh_load_kernel( * split into smaller allocations, done as a single region in order to * simplify it. */ - last_addr = find_memory(d, &elf, sizeof(start_info) + - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + - sizeof(mod) - : 0) + - (cmdline ? ROUNDUP(strlen(cmdline) + 1, - elf_64bit(&elf) ? 8 : 4) - : 0)); + metadata_len = sizeof(start_info); + + if ( initrd ) + metadata_len += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); + + if ( cmdline ) + metadata_len += ROUNDUP(strlen(cmdline) + 1, + elf_64bit(&elf) ? 8 : 4); + + last_addr = find_memory(d, &elf, metadata_len); if ( last_addr == INVALID_PADDR ) { printk("Unable to find a memory region to load initrd and metadata\n"); which I think I ought to submit as a prerequisite to this, after which your logic squashed on the RHS now becomes an expansion of the `if ( initrd )`. Thoughts? ~Andrew
On Wed, Mar 19, 2025 at 12:12:22PM +0000, Andrew Cooper wrote: > On 19/03/2025 11:18 am, 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. > > > > Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > --- a/xen/arch/x86/hvm/dom0_build.c > > +++ b/xen/arch/x86/hvm/dom0_build.c > > @@ -712,7 +712,15 @@ static int __init pvh_load_kernel( > > * simplify it. > > */ > > last_addr = find_memory(d, &elf, sizeof(start_info) + > > - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + > > + (initrd ? ROUNDUP(ROUNDUP(initrd_len, > > + elf_64bit(&elf) ? 8 : 4) + > > + (initrd->cmdline_pa && > > + strlen(__va(initrd-> > > + cmdline_pa)) > > + ? strlen(__va(initrd-> > > + cmdline_pa)) + 1 > > + : 0), > > + PAGE_SIZE) + > > sizeof(mod) > > : 0) + > > (cmdline ? ROUNDUP(strlen(cmdline) + 1, > > This piece of logic was already bad, but this is rather worse. > > One patch I made while doing the boot module work is: > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index 6a4453103a9a..7292ddd07276 100644 > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -654,6 +654,7 @@ static int __init pvh_load_kernel( > const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : > NULL; > struct elf_binary elf; > struct elf_dom_parms parms; > + size_t metadata_len; > paddr_t last_addr; > struct hvm_start_info start_info = { 0 }; > struct hvm_modlist_entry mod = { 0 }; > @@ -711,13 +712,16 @@ static int __init pvh_load_kernel( > * split into smaller allocations, done as a single region in order to > * simplify it. > */ > - last_addr = find_memory(d, &elf, sizeof(start_info) + > - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + > - sizeof(mod) > - : 0) + > - (cmdline ? ROUNDUP(strlen(cmdline) + 1, > - elf_64bit(&elf) ? 8 : 4) > - : 0)); > + metadata_len = sizeof(start_info); > + > + if ( initrd ) > + metadata_len += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); > + > + if ( cmdline ) > + metadata_len += ROUNDUP(strlen(cmdline) + 1, > + elf_64bit(&elf) ? 8 : 4); > + > + last_addr = find_memory(d, &elf, metadata_len); > if ( last_addr == INVALID_PADDR ) > { > printk("Unable to find a memory region to load initrd and > metadata\n"); > > > which I think I ought to submit as a prerequisite to this, after which > your logic squashed on the RHS now becomes an expansion of the `if ( > initrd )`. > > Thoughts? Yes please, I was going to ask Jan to break that chain of ?: operators as it's impossible to read. Thanks, Roger.
On 19.03.2025 13:12, Andrew Cooper wrote: > On 19/03/2025 11:18 am, 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. >> >> Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/hvm/dom0_build.c >> +++ b/xen/arch/x86/hvm/dom0_build.c >> @@ -712,7 +712,15 @@ static int __init pvh_load_kernel( >> * simplify it. >> */ >> last_addr = find_memory(d, &elf, sizeof(start_info) + >> - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + >> + (initrd ? ROUNDUP(ROUNDUP(initrd_len, >> + elf_64bit(&elf) ? 8 : 4) + >> + (initrd->cmdline_pa && >> + strlen(__va(initrd-> >> + cmdline_pa)) >> + ? strlen(__va(initrd-> >> + cmdline_pa)) + 1 >> + : 0), >> + PAGE_SIZE) + >> sizeof(mod) >> : 0) + >> (cmdline ? ROUNDUP(strlen(cmdline) + 1, > > This piece of logic was already bad, but this is rather worse. > > One patch I made while doing the boot module work is: > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c > index 6a4453103a9a..7292ddd07276 100644 > --- a/xen/arch/x86/hvm/dom0_build.c > +++ b/xen/arch/x86/hvm/dom0_build.c > @@ -654,6 +654,7 @@ static int __init pvh_load_kernel( > const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : > NULL; > struct elf_binary elf; > struct elf_dom_parms parms; > + size_t metadata_len; > paddr_t last_addr; > struct hvm_start_info start_info = { 0 }; > struct hvm_modlist_entry mod = { 0 }; > @@ -711,13 +712,16 @@ static int __init pvh_load_kernel( > * split into smaller allocations, done as a single region in order to > * simplify it. > */ > - last_addr = find_memory(d, &elf, sizeof(start_info) + > - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + > - sizeof(mod) > - : 0) + > - (cmdline ? ROUNDUP(strlen(cmdline) + 1, > - elf_64bit(&elf) ? 8 : 4) > - : 0)); > + metadata_len = sizeof(start_info); > + > + if ( initrd ) > + metadata_len += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); > + > + if ( cmdline ) > + metadata_len += ROUNDUP(strlen(cmdline) + 1, > + elf_64bit(&elf) ? 8 : 4); > + > + last_addr = find_memory(d, &elf, metadata_len); > if ( last_addr == INVALID_PADDR ) > { > printk("Unable to find a memory region to load initrd and > metadata\n"); > > > which I think I ought to submit as a prerequisite to this, after which > your logic squashed on the RHS now becomes an expansion of the `if ( > initrd )`. Yes please. Another thing I was considering to put in a local variable is the repeated "elf_64bit(&elf) ? 8 : 4". If that sounds okay, I can make that a(nother) pre-patch to mine. Jan
On 19/03/2025 12:39 pm, Jan Beulich wrote: > On 19.03.2025 13:12, Andrew Cooper wrote: >> On 19/03/2025 11:18 am, 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. >>> >>> Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> --- a/xen/arch/x86/hvm/dom0_build.c >>> +++ b/xen/arch/x86/hvm/dom0_build.c >>> @@ -712,7 +712,15 @@ static int __init pvh_load_kernel( >>> * simplify it. >>> */ >>> last_addr = find_memory(d, &elf, sizeof(start_info) + >>> - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + >>> + (initrd ? ROUNDUP(ROUNDUP(initrd_len, >>> + elf_64bit(&elf) ? 8 : 4) + >>> + (initrd->cmdline_pa && >>> + strlen(__va(initrd-> >>> + cmdline_pa)) >>> + ? strlen(__va(initrd-> >>> + cmdline_pa)) + 1 >>> + : 0), >>> + PAGE_SIZE) + >>> sizeof(mod) >>> : 0) + >>> (cmdline ? ROUNDUP(strlen(cmdline) + 1, >> This piece of logic was already bad, but this is rather worse. >> >> One patch I made while doing the boot module work is: >> >> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c >> index 6a4453103a9a..7292ddd07276 100644 >> --- a/xen/arch/x86/hvm/dom0_build.c >> +++ b/xen/arch/x86/hvm/dom0_build.c >> @@ -654,6 +654,7 @@ static int __init pvh_load_kernel( >> const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) : >> NULL; >> struct elf_binary elf; >> struct elf_dom_parms parms; >> + size_t metadata_len; >> paddr_t last_addr; >> struct hvm_start_info start_info = { 0 }; >> struct hvm_modlist_entry mod = { 0 }; >> @@ -711,13 +712,16 @@ static int __init pvh_load_kernel( >> * split into smaller allocations, done as a single region in order to >> * simplify it. >> */ >> - last_addr = find_memory(d, &elf, sizeof(start_info) + >> - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + >> - sizeof(mod) >> - : 0) + >> - (cmdline ? ROUNDUP(strlen(cmdline) + 1, >> - elf_64bit(&elf) ? 8 : 4) >> - : 0)); >> + metadata_len = sizeof(start_info); >> + >> + if ( initrd ) >> + metadata_len += sizeof(mod) + ROUNDUP(initrd_len, PAGE_SIZE); >> + >> + if ( cmdline ) >> + metadata_len += ROUNDUP(strlen(cmdline) + 1, >> + elf_64bit(&elf) ? 8 : 4); >> + >> + last_addr = find_memory(d, &elf, metadata_len); >> if ( last_addr == INVALID_PADDR ) >> { >> printk("Unable to find a memory region to load initrd and >> metadata\n"); >> >> >> which I think I ought to submit as a prerequisite to this, after which >> your logic squashed on the RHS now becomes an expansion of the `if ( >> initrd )`. > Yes please. https://lore.kernel.org/xen-devel/20250319171346.19478-1-andrew.cooper3@citrix.com/T/#u > Another thing I was considering to put in a local variable is the > repeated "elf_64bit(&elf) ? 8 : 4". If that sounds okay, I can make that > a(nother) pre-patch to mine. Yeah, I was thinking the same. There's a related expression in elf_round_up(), so it might be worth having a macro in include/xen/libelf.h for elf_ptr_size() or so. ~Andrew
--- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -712,7 +712,15 @@ static int __init pvh_load_kernel( * simplify it. */ last_addr = find_memory(d, &elf, sizeof(start_info) + - (initrd ? ROUNDUP(initrd_len, PAGE_SIZE) + + (initrd ? ROUNDUP(ROUNDUP(initrd_len, + elf_64bit(&elf) ? 8 : 4) + + (initrd->cmdline_pa && + strlen(__va(initrd-> + cmdline_pa)) + ? strlen(__va(initrd-> + cmdline_pa)) + 1 + : 0), + PAGE_SIZE) + sizeof(mod) : 0) + (cmdline ? ROUNDUP(strlen(cmdline) + 1, @@ -740,16 +748,19 @@ static int __init pvh_load_kernel( if ( initrd->cmdline_pa ) { char *str = __va(initrd->cmdline_pa); - size_t len = strlen(str) + 1; + size_t len = strlen(str); - rc = hvm_copy_to_guest_phys(last_addr, str, len, v); - if ( rc ) + if ( len++ ) { - printk("Unable to copy module command line\n"); - return rc; + rc = hvm_copy_to_guest_phys(last_addr, str, len, v); + if ( rc ) + { + printk("Unable to copy module command line\n"); + return rc; + } + mod.cmdline_paddr = last_addr; + last_addr += len; } - mod.cmdline_paddr = last_addr; - last_addr += len; } last_addr = ROUNDUP(last_addr, PAGE_SIZE); }
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. Fixes: 0ecb8eb09f9f ("x86/pvh: pass module command line to dom0") Signed-off-by: Jan Beulich <jbeulich@suse.com>