| Message ID | 20230323161053.412356-2-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix dumpdtb crash with ARM machines | expand |
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
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
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
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
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;
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(-)