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

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

[Daniel Henrique Barboza]

Hi,

This is a re-post of "[PATCH v8 03/16] hw/arm: do not free machine->fdt
in arm_load_dtb()":

https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04201.html

Turns out that I drop the ball and left this patch behind. Aside from
some patches of that series that were optional, the way ARM code is
working ATM is causing 'dumpdtb' to crash QEMU, as reported by Markus in
https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg05858.html .

Applying this patch fixes the reported crash:

$ ./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-g298c4469cf"}, "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": 1679587153, "microseconds": 714319}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-signal"}}
$ 
$ dtc -I dtb -O dts fdt.dtb | grep timer
	timer {
		compatible = "arm,armv7-timer";
$

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH 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.

One alternative would be to mark machine->fdt = NULL when exiting
arm_load_dtb() when freeing the fdt. Another is to not free the fdt and,
instead, update machine->fdt with the new fdt generated. This will
enable dumpdtb for all ARM machines that uses arm_load_dtb(), regardless
of having 'dtb_filename' or not.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 50e5141116..9418cc3373 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -689,7 +689,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 1/1] hw/arm: do not free machine->fdt in arm_load_dtb()

[Peter Maydell]

On Thu, 23 Mar 2023 at 16:11, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
> 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.
>
> One alternative would be to mark machine->fdt = NULL when exiting
> arm_load_dtb() when freeing the fdt. Another is to not free the fdt and,
> instead, update machine->fdt with the new fdt generated. This will
> enable dumpdtb for all ARM machines that uses arm_load_dtb(), regardless
> of having 'dtb_filename' or not.
>
> 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 50e5141116..9418cc3373 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -689,7 +689,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;

With this we're now setting ms->fdt twice for the virt board: we set
it in create_fdt() in hw/arm/virt.c, and then we set it again here.
Which is the right place to set it?

Is the QMP 'dumpdtb' command intended to dump the DTB only for
board types where the DTB is created at runtime by QEMU? Or
is it supposed to also work for DTBs that were originally
provided by the user using the '-dtb' command line? The docs
don't say. If we want the former, then we should be setting
ms->fdt in the board code; if the latter, then here is right.

thanks
-- PMM

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

[Daniel Henrique Barboza]

On 3/23/23 14:38, Peter Maydell wrote:
> On Thu, 23 Mar 2023 at 16:11, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>> 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.
>>
>> One alternative would be to mark machine->fdt = NULL when exiting
>> arm_load_dtb() when freeing the fdt. Another is to not free the fdt and,
>> instead, update machine->fdt with the new fdt generated. This will
>> enable dumpdtb for all ARM machines that uses arm_load_dtb(), regardless
>> of having 'dtb_filename' or not.
>>
>> 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 | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 50e5141116..9418cc3373 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -689,7 +689,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;
> 
> With this we're now setting ms->fdt twice for the virt board: we set
> it in create_fdt() in hw/arm/virt.c, and then we set it again here.
> Which is the right place to set it?
> 
> Is the QMP 'dumpdtb' command intended to dump the DTB only for
> board types where the DTB is created at runtime by QEMU? Or
> is it supposed to also work for DTBs that were originally
> provided by the user using the '-dtb' command line? The docs
> don't say. If we want the former, then we should be setting
> ms->fdt in the board code; if the latter, then here is right.

My original intent with this command was to dump the current state of the FDT,
regardless of whether the FDT was loaded via -dtb or at runtime.

Ideally it would also reflect hotplug changes that affects the FDT as well, although
I'm aware of only one board that does that (ppc64 pseries) and we would need some
work done that to update ms->fdt after the hotplug/hotunplug path.

Perhaps a doc path would also be good.


Daniel

> 
> thanks
> -- PMM

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

[Peter Maydell]

On Thu, 23 Mar 2023 at 17:54, Daniel Henrique Barboza
<danielhb413@gmail.com> wrote:
>
>
>
> On 3/23/23 14:38, Peter Maydell wrote:
> > On Thu, 23 Mar 2023 at 16:11, Daniel Henrique Barboza
> > <danielhb413@gmail.com> wrote:
> >> -    g_free(fdt);
> >> +    /* Set ms->fdt for 'dumpdtb' QMP/HMP command */
> >> +    ms->fdt = fdt;
> >
> > With this we're now setting ms->fdt twice for the virt board: we set
> > it in create_fdt() in hw/arm/virt.c, and then we set it again here.
> > Which is the right place to set it?
> >
> > Is the QMP 'dumpdtb' command intended to dump the DTB only for
> > board types where the DTB is created at runtime by QEMU? Or
> > is it supposed to also work for DTBs that were originally
> > provided by the user using the '-dtb' command line? The docs
> > don't say. If we want the former, then we should be setting
> > ms->fdt in the board code; if the latter, then here is right.
>
> My original intent with this command was to dump the current state of the FDT,
> regardless of whether the FDT was loaded via -dtb or at runtime.

Mmm. I think that makes sense; we do make a few tweaks to the DTB
even if it was user-provided and you might want to check those
for debug purposes. So we should keep this assignment, and remove
the now-unneeded setting of ms->fdt in create_fdt().

thanks
-- PMM

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

[Daniel Henrique Barboza]

On 3/23/23 14:59, Peter Maydell wrote:
> On Thu, 23 Mar 2023 at 17:54, Daniel Henrique Barboza
> <danielhb413@gmail.com> wrote:
>>
>>
>>
>> On 3/23/23 14:38, Peter Maydell wrote:
>>> On Thu, 23 Mar 2023 at 16:11, Daniel Henrique Barboza
>>> <danielhb413@gmail.com> wrote:
>>>> -    g_free(fdt);
>>>> +    /* Set ms->fdt for 'dumpdtb' QMP/HMP command */
>>>> +    ms->fdt = fdt;
>>>
>>> With this we're now setting ms->fdt twice for the virt board: we set
>>> it in create_fdt() in hw/arm/virt.c, and then we set it again here.
>>> Which is the right place to set it?
>>>
>>> Is the QMP 'dumpdtb' command intended to dump the DTB only for
>>> board types where the DTB is created at runtime by QEMU? Or
>>> is it supposed to also work for DTBs that were originally
>>> provided by the user using the '-dtb' command line? The docs
>>> don't say. If we want the former, then we should be setting
>>> ms->fdt in the board code; if the latter, then here is right.
>>
>> My original intent with this command was to dump the current state of the FDT,
>> regardless of whether the FDT was loaded via -dtb or at runtime.
> 
> Mmm. I think that makes sense; we do make a few tweaks to the DTB
> even if it was user-provided and you might want to check those
> for debug purposes. So we should keep this assignment, and remove
> the now-unneeded setting of ms->fdt in create_fdt().


I don't think we can remove it. arm_load_dtb() does the following:

     if (binfo->dtb_filename) {
        (...)
     } else {
         fdt = binfo->get_dtb(binfo, &size);
         if (!fdt) {
             fprintf(stderr, "Board was unable to create a dtb blob\n");
             goto fail;
         }
     }

So if we don't have a '-dtb' option, fdt = binfo->get_dtb(). For the 'virt' machine,
machvirt_dtb(), will return ms->fdt. So we would SIGSEG right at the start.

And now that I think more about it, this patch is leaking the board FDT if we're
using the FDT from dtb_filename, isn't it? We're assigning a new ms->fdt on top
of the existing ms->fdt from the board. I'll send a new version.

Also, given that we're not using the board FDT at all if '-dtb' is present, I
think it would be good to move create_fdt() from machvirt_init() to machvirt_dtb().
Some code juggling would be required (some functions from init() are using ms->fdt)
but it think it would make the code clearer - create the fdt board only if we're
really going to use it. I'll see if I can pull this off and send a 8.1 patch
with it.


Thanks,


Daniel




> 
> thanks
> -- PMM