[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/PVH: account for module command line length
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@xxxxxxxx> >>> >>> --- 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@xxxxxxxxxx/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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |