Re: QMP command dumpdtb crash bug

[Daniel Henrique Barboza]

On 3/23/23 03:29, Markus Armbruster wrote:
> Watch this:
>
>      $ gdb --args ../qemu/bld/qemu-system-aarch64 -S -M virt -display none -qmp 
> stdio
>      [...]
>      (gdb) r
>      [...]
>      {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 7}, "package": 
> "v7.2.0-2331-gda89f78a7d"}, "capabilities": ["oob"]}}
>      [New Thread 0x7fffed62c6c0 (LWP 1021967)]
>      {"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
>      {"return": {}}
>      {"execute": "dumpdtb", "arguments": {"filename": "fdt.dtb"}}
>
>      Thread 1 "qemu-system-aar" received signal SIGSEGV, Segmentation fault.
>      qmp_dumpdtb (filename=0x5555581c5170 "fdt.dtb", 
> errp=errp@entry=0x7fffffffdae8)
>          at ../softmmu/device_tree.c:661
>      661            size = fdt_totalsize(current_machine->fdt);
>
> current_machine->fdt is non-null here.  The crash is within
> fdt_totalsize().


Back when I added this command [1] one of the patches of this series was:

[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

The patch aimed to address what you're seeing here. machine->fdt is not NULL,
but it was freed in arm_load_dtb() without assigning it to NULL and it contains
junk.

Back then this patch got no acks/reviews and got left behind. If I apply it now
at current master your example works.

> I suspect ...
>
>      void qmp_dumpdtb(const char *filename, Error **errp)
>      {
>          g_autoptr(GError) err = NULL;
>          uint32_t size;
>
>          if (!current_machine->fdt) {
>              error_setg(errp, "This machine doesn't have a FDT");
>              return;
>          }
>
> ... we're missing an "FDT isn't ready" guard here.


There might be a case where a guard would be needed, but for all ARM machines 
that
uses arm_load_dtb() I can say that the dumpdtb is broken regardless of whether 
you
attempt it during early boot or not.

One solution is to just apply the patch I mentioned above. Another solution is 
to
make a g_free(fdt) in arm_load_dtb and also do a ms->fdt = NULL to tell 
dumpdtb()
that there is no fdt available.

I don't see any particular reason why arm machines can't support this command, 
so
let me know and I can re-send that patch.



Thanks,


Daniel


[1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg04190.html

>          size = fdt_totalsize(current_machine->fdt);
>
>          g_assert(size > 0);
>
>          if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) {
>              error_setg(errp, "Error saving FDT to file %s: %s",
>                         filename, err->message);
>          }
>      }
>
> Also, I think the error message "does not have a FDT" should say "an
> FDT".