Re: QMP command dumpdtb crash bug

[Markus Armbruster <armbru@redhat.com>]

Peter, Daniel offers two ways to fix this bug (see below).  Got a
preference?

Daniel Henrique Barboza <dbarboza@ventanamicro.com> writes:

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