[PATCH v2 0/1] fix dumpdtb crash with ARM machines

[PATCH v2 0/1] fix dumpdtb crash with ARM machines

[Daniel Henrique Barboza]

Hi,

In this version I fixed a mem leak that was happening if the user inputs
a fdt via '-dtb'. In that case we would assign the updated FDT on top of
the existing board FDT that was already assigned to ms->fdt.

Tested as follows:

$ ./qemu-system-aarch64 -S -M virt -display none -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 91, "minor": 2, "major": 7}, "package": "v8.0.0-rc1-37-ge573ef31e7-dirty"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
{"return": {}}

{"execute": "dumpdtb", "arguments": {"filename": "fdt.dtb"}}
{"return": {}}

^Cqemu-system-aarch64: terminating on signal 2
{"timestamp": {"seconds": 1679603324, "microseconds": 62159}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-signal"}}
$ 


$ ./qemu-system-aarch64 -S -M virt -display none -qmp stdio -dtb fdt.dtb 
{"QMP": {"version": {"qemu": {"micro": 91, "minor": 2, "major": 7}, "package": "v8.0.0-rc1-37-ge573ef31e7-dirty"}, "capabilities": ["oob"]}}
{"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
{"return": {}}

{"execute": "dumpdtb", "arguments": {"filename": "fdt.dtb"}}
{"return": {}}
^Cqemu-system-aarch64: terminating on signal 2
{"timestamp": {"seconds": 1679603344, "microseconds": 145748}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-signal"}}
$ 


First we use 'dumpdtb' to dump the board FDT in fdt.dtb, then we use it as
an argument to '-dtb' and do the test again. This covers both code paths.


Changes from v1:
- g_free(ms->fdt) before load_device_tree()
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg05930.html

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: qemu-arm@nongnu.org

Daniel Henrique Barboza (1):
  hw/arm: do not free machine->fdt in arm_load_dtb()

 hw/arm/boot.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

-- 
2.39.2

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

[Daniel Henrique Barboza]

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);
+
         fdt = load_device_tree(filename, &size);
         if (!fdt) {
             fprintf(stderr, "Couldn't open dtb file %s\n", filename);
@@ -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;
 
-- 
2.39.2

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

[Markus Armbruster]

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?

>          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?

       }

> @@ -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;

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

[Daniel Henrique Barboza]

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'.

> 
>>           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).


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.


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;
> 

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

[Markus Armbruster]

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;
 

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

[Daniel Henrique Barboza]

On 3/28/23 06:53, Markus Armbruster wrote:
> 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;

This looks better than what I've been proposing here because it centers everything in
the same spot. It'll also make it easier to change/remove it when we have the chance
to take a look at the ARM boot code.

Just tested it here and it works fine. Feel free to format it into a patch and send
it. I'll give my r-b.


Thanks,

Daniel

>   
> 

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

[Markus Armbruster]

Daniel Henrique Barboza <danielhb413@gmail.com> writes:

> On 3/28/23 06:53, Markus Armbruster wrote:
>> Daniel Henrique Barboza <danielhb413@gmail.com> writes:

[...]

>>> 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;
>
> This looks better than what I've been proposing here because it centers everything in
> the same spot. It'll also make it easier to change/remove it when we have the chance
> to take a look at the ARM boot code.
>
> Just tested it here and it works fine. Feel free to format it into a patch and send
> it. I'll give my r-b.

I'm going to send it as v3 with your S-o-B, because I'm stealing most of
your commit message.