Daniel Henrique Barboza <danielhb...@gmail.com> writes: > On 3/28/23 04:01, Markus Armbruster wrote: >> Daniel Henrique Barboza <danielhb...@gmail.com> writes: >> >>> At this moment, arm_load_dtb() can free machine->fdt when >>> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be >>> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is >>> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to >>> machine->fdt. And, in that case, the existing g_free(fdt) at the end of >>> arm_load_dtb() will make machine->fdt point to an invalid memory region. >>> >>> After the command 'dumpdtb' were introduced a couple of releases ago, >>> running it with any ARM machine that uses arm_load_dtb() will crash >>> QEMU. >>> >>> Let's enable all arm_load_dtb() callers to use dumpdtb properly. Instead >>> of freeing 'fdt', assign it back to ms->fdt. >>> >>> Note that all current callers (sbsa-ref.c, virt.c, xlnx-versal-virt.c) >>> are assigning ms->fdt before arm_load_dtb() is called, regardless of >>> whether the user is inputting an external FDT via '-dtb'. To avoid >>> leaking the board FDT if '-dtb' is used (since we're assigning ms->fdt >>> in the end), free ms->fdt before load_device_tree(). >>> >>> Cc: Peter Maydell <peter.mayd...@linaro.org> >>> Cc: qemu-...@nongnu.org >>> Fixes: bf353ad55590f ("qmp/hmp, device_tree.c: introduce dumpdtb") >>> Reported-by: Markus Armbruster <arm...@redhat.com>i >>> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com> >>> --- >>> hw/arm/boot.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >>> index 50e5141116..de18c0a969 100644 >>> --- a/hw/arm/boot.c >>> +++ b/hw/arm/boot.c >>> @@ -549,6 +549,13 @@ int arm_load_dtb(hwaddr addr, const struct >>> arm_boot_info *binfo, >>> goto fail; >>> } >>> >>> + /* >>> + * If we're here we won't be using the ms->fdt from the board. >>> + * We'll assign a new ms->fdt at the end, so free it now to >>> + * avoid leaking the board FDT. >>> + */ >>> + g_free(ms->fdt); >>> + >> >> "We will" is not true: we will not if we goto fail. Leaves ms->fdt >> dangling, doesn't it? > > We can postpone this g_free() to execute after "if (!fdt) {}" to be sure that > we're > not freeing ms->fdt right before 'goto fail'.
Yes, but what about all the goto fail further down? >>> fdt = load_device_tree(filename, &size); >>> if (!fdt) { >>> fprintf(stderr, "Couldn't open dtb file %s\n", filename); >> g_free(filename); >> goto fail; >> } >> g_free(filename); >> } else { >> fdt = binfo->get_dtb(binfo, &size); >> if (!fdt) { >> fprintf(stderr, "Board was unable to create a dtb blob\n"); >> goto fail; >> } >> >> If we succeed, we'll assign @fdt to ms->fdt (next hunk). Won't this >> leak old ms->fdt? > > > For all callers binfo->get_dtb() is returning ms->fdt, i.e. this line: > > fdt = binfo->get_dtb(binfo, &size); > > Is equal to this: > > fdt = ms->fdt; > > And this is why we can't unconditionally do a g_free(ms->fdt). Uff. Not exactly obvious. > I believe we can improve the ARM boot code to not create ms->fdt at init(), > leaving it unassigned, and make get_dtb() return the machine FDT on a common > "void *" pointer. That would spare us from having go g_free(ms->fdt) to avoid > leaks and we would assign ms->fdt at the end of arm_load_dtb() normally. I > made > a quick attempt at that but the ARM init() code is a little tricker than I've > anticipated. I might have a crack at it later. Do we want a quick interim fix for 8.0? Have a careful look at the untested patch below. > Thanks, > > Daniel > > >> >> } >> >>> @@ -689,7 +696,8 @@ int arm_load_dtb(hwaddr addr, const struct >>> arm_boot_info *binfo, >>> qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds, >>> rom_ptr_for_as(as, addr, size)); >>> >>> - g_free(fdt); >>> + /* Set ms->fdt for 'dumpdtb' QMP/HMP command */ >>> + ms->fdt = fdt; >>> >>> return size; >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c index 50e5141116..54f6a3e0b3 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -689,7 +689,10 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo, qemu_register_reset_nosnapshotload(qemu_fdt_randomize_seeds, rom_ptr_for_as(as, addr, size)); - g_free(fdt); + if (fdt != ms->fdt) { + g_free(ms->fdt); + ms->fdt = fdt; + } return size;