[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 1/1] hw/arm: do not free machine->fdt in arm_load_dtb()
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 1/1] hw/arm: do not free machine->fdt in arm_load_dtb() |
Date: |
Tue, 28 Mar 2023 11:53:32 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> On 3/28/23 04:01, Markus Armbruster wrote:
>> Daniel Henrique Barboza <danielhb413@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.maydell@linaro.org>
>>> Cc: qemu-arm@nongnu.org
>>> Fixes: bf353ad55590f ("qmp/hmp, device_tree.c: introduce dumpdtb")
>>> Reported-by: Markus Armbruster <armbru@redhat.com>i
>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@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;