Re: [PATCH v2 1/1] hw/arm: do not free machine->fdt in arm_load_dtb()

[Markus Armbruster <armbru@redhat.com>]

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;