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