* [PATCH 0/2] EFI: adjustments after "Unified Xen hypervisor/kernel/initrd images" @ 2020-10-14 10:40 Jan Beulich 2020-10-14 10:42 ` [PATCH 1/2] EFI/Arm64: don't clobber DTB pointer Jan Beulich 2020-10-14 10:42 ` [PATCH 2/2] EFI: further "need_to_free" adjustments Jan Beulich 0 siblings, 2 replies; 5+ messages in thread From: Jan Beulich @ 2020-10-14 10:40 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Trammell Hudson The first change is, I believe, addressing the regression spotted by osstest. The second change is simply a result of me going over the involved code in, effectively, a re-review of the original changes. 1: EFI/Arm64: don't clobber DTB pointer 2: EFI: further "need_to_free" adjustments Jan ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] EFI/Arm64: don't clobber DTB pointer 2020-10-14 10:40 [PATCH 0/2] EFI: adjustments after "Unified Xen hypervisor/kernel/initrd images" Jan Beulich @ 2020-10-14 10:42 ` Jan Beulich 2020-10-14 10:57 ` Andrew Cooper 2020-10-14 10:42 ` [PATCH 2/2] EFI: further "need_to_free" adjustments Jan Beulich 1 sibling, 1 reply; 5+ messages in thread From: Jan Beulich @ 2020-10-14 10:42 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Trammell Hudson read_section() needs to be more careful: efi_arch_use_config_file() may have found a DTB file (but without modules), and there may be no DTB specified in the EFI config file. In this case the pointer to the blob must not be overwritten with NULL when no ".dtb" section is present either. Fixes: 8a71d50ed40b ("efi: Enable booting unified hypervisor/kernel/initrd images") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -637,11 +637,14 @@ static bool __init read_section(const EF const CHAR16 *name, struct file *file, const char *options) { - file->ptr = pe_find_section(image->ImageBase, image->ImageSize, - name, &file->size); - if ( !file->ptr ) + const void *ptr = pe_find_section(image->ImageBase, image->ImageSize, + name, &file->size); + + if ( !ptr ) return false; + file->ptr = ptr; + handle_file_info(name, file, options); return true; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] EFI/Arm64: don't clobber DTB pointer 2020-10-14 10:42 ` [PATCH 1/2] EFI/Arm64: don't clobber DTB pointer Jan Beulich @ 2020-10-14 10:57 ` Andrew Cooper 0 siblings, 0 replies; 5+ messages in thread From: Andrew Cooper @ 2020-10-14 10:57 UTC (permalink / raw) To: Jan Beulich, xen-devel Cc: George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Trammell Hudson On 14/10/2020 11:42, Jan Beulich wrote: > read_section() needs to be more careful: efi_arch_use_config_file() > may have found a DTB file (but without modules), and there may be no DTB > specified in the EFI config file. In this case the pointer to the blob > must not be overwritten with NULL when no ".dtb" section is present > either. > > Fixes: 8a71d50ed40b ("efi: Enable booting unified hypervisor/kernel/initrd images") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] EFI: further "need_to_free" adjustments 2020-10-14 10:40 [PATCH 0/2] EFI: adjustments after "Unified Xen hypervisor/kernel/initrd images" Jan Beulich 2020-10-14 10:42 ` [PATCH 1/2] EFI/Arm64: don't clobber DTB pointer Jan Beulich @ 2020-10-14 10:42 ` Jan Beulich 2020-10-14 20:15 ` Stefano Stabellini 1 sibling, 1 reply; 5+ messages in thread From: Jan Beulich @ 2020-10-14 10:42 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Trammell Hudson When processing "chain" directives, the previously loaded config file gets freed. This needs to be recorded accordingly such that no error path would try to free the same block of memory a 2nd time. Furthermore, neither .addr nor .size being zero has any meaning towards the need to free an allocated chunk anymore. Drop (from read_file()) and replace (in Arm's efi_arch_use_config_file(), to sensibly retain the comment) respective assignments. Fixes: 04be2c3a0678 ("efi/boot.c: add file.need_to_free") Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/arm/efi/efi-boot.h +++ b/xen/arch/arm/efi/efi-boot.h @@ -591,7 +591,7 @@ static bool __init efi_arch_use_config_f fdt = lookup_fdt_config_table(SystemTable); dtbfile.ptr = fdt; - dtbfile.size = 0; /* Config table memory can't be freed, so set size to 0 */ + dtbfile.need_to_free = false; /* Config table memory can't be freed. */ if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 ) { /* --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -601,10 +601,7 @@ static bool __init read_file(EFI_FILE_HA PFN_UP(size), &file->addr); } if ( EFI_ERROR(ret) ) - { - file->addr = 0; what = what ?: L"Allocation"; - } else { file->need_to_free = true; @@ -1271,8 +1268,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY name.s = get_value(&cfg, "global", "chain"); if ( !name.s ) break; - efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); - cfg.addr = 0; + if ( cfg.need_to_free ) + { + efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); + cfg.need_to_free = false; + } if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) ) { PrintStr(L"Chained configuration file '"); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] EFI: further "need_to_free" adjustments 2020-10-14 10:42 ` [PATCH 2/2] EFI: further "need_to_free" adjustments Jan Beulich @ 2020-10-14 20:15 ` Stefano Stabellini 0 siblings, 0 replies; 5+ messages in thread From: Stefano Stabellini @ 2020-10-14 20:15 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall, Wei Liu, Stefano Stabellini, Trammell Hudson On Wed, 14 Oct 2020, Jan Beulich wrote: > When processing "chain" directives, the previously loaded config file > gets freed. This needs to be recorded accordingly such that no error > path would try to free the same block of memory a 2nd time. > > Furthermore, neither .addr nor .size being zero has any meaning towards > the need to free an allocated chunk anymore. Drop (from read_file()) and > replace (in Arm's efi_arch_use_config_file(), to sensibly retain the > comment) respective assignments. > > Fixes: 04be2c3a0678 ("efi/boot.c: add file.need_to_free") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- a/xen/arch/arm/efi/efi-boot.h > +++ b/xen/arch/arm/efi/efi-boot.h > @@ -591,7 +591,7 @@ static bool __init efi_arch_use_config_f > > fdt = lookup_fdt_config_table(SystemTable); > dtbfile.ptr = fdt; > - dtbfile.size = 0; /* Config table memory can't be freed, so set size to 0 */ > + dtbfile.need_to_free = false; /* Config table memory can't be freed. */ > if ( !fdt || fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") < 0 ) > { > /* > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -601,10 +601,7 @@ static bool __init read_file(EFI_FILE_HA > PFN_UP(size), &file->addr); > } > if ( EFI_ERROR(ret) ) > - { > - file->addr = 0; > what = what ?: L"Allocation"; > - } > else > { > file->need_to_free = true; > @@ -1271,8 +1268,11 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SY > name.s = get_value(&cfg, "global", "chain"); > if ( !name.s ) > break; > - efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > - cfg.addr = 0; > + if ( cfg.need_to_free ) > + { > + efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size)); > + cfg.need_to_free = false; > + } > if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) ) > { > PrintStr(L"Chained configuration file '"); > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-14 20:15 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-14 10:40 [PATCH 0/2] EFI: adjustments after "Unified Xen hypervisor/kernel/initrd images" Jan Beulich 2020-10-14 10:42 ` [PATCH 1/2] EFI/Arm64: don't clobber DTB pointer Jan Beulich 2020-10-14 10:57 ` Andrew Cooper 2020-10-14 10:42 ` [PATCH 2/2] EFI: further "need_to_free" adjustments Jan Beulich 2020-10-14 20:15 ` Stefano Stabellini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).