[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion
From: |
Peter Maydell |
Subject: |
Re: [PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion |
Date: |
Tue, 26 Sep 2023 15:33:58 +0100 |
On Tue, 26 Sept 2023 at 15:20, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 26.09.23 13:51, Peter Maydell wrote:
> > On Mon, 25 Sept 2023 at 20:42, Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> Coverity mark this size, got from the buffer as untrasted value, it's
> >> not good to use it as length when writing to file. Make the assertion
> >> more strict to also check upper bound.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> ---
> >> softmmu/device_tree.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/softmmu/device_tree.c b/softmmu/device_tree.c
> >> index 30aa3aea9f..adc4236e21 100644
> >> --- a/softmmu/device_tree.c
> >> +++ b/softmmu/device_tree.c
> >> @@ -660,7 +660,7 @@ void qmp_dumpdtb(const char *filename, Error **errp)
> >>
> >> size = fdt_totalsize(current_machine->fdt);
> >>
> >> - g_assert(size > 0);
> >> + g_assert(size > 0 && size <= FDT_MAX_SIZE);
> >
> > FDT_MAX_SIZE is not "this is as big as an FDT can ever be". It's
> > only the internal sizing of device trees that we create ourselves
> > in the machine models (and which we will bump up if for some
> > reason we ever find ourselves needing to create bigger device
> > trees). So it's not really a suitable upper bound.
> >
> >> if (!g_file_set_contents(filename, current_machine->fdt, size,
> >> &err)) {
> >> error_setg(errp, "Error saving FDT to file %s: %s",
> >
> > Nothing bad happens if we pass g_file_set_contents() a very
>
> but it will also try to read beyond the allocated fdt.
No, it won't, because we can assume that current_machine->fdt points
to a valid device tree blob, and so size is by definition correct.
The libfdt code keeps track of the size of the memory we allocated
for it to use -- when we call fdt_open_into() we pass that size.
fdt_totalsize() simply returns that passed in size value. So
the amount of memory we can access is exactly "size".
(The fdt may not have come from create_device_tree(), so
it's possible both that the size as returned by fdt_totalsize()
can validly be larger than FDT_MAX_SIZE, and also that the fdt
blob can be smaller than FDT_MAX_SIZE. So that's the wrong thing
to try to check against either way.)
thanks
-- PMM
- Re: [PATCH 01/12] hw/core/loader: load_at(): check size, (continued)
[PATCH 03/12] util/filemonitor-inotify: qemu_file_monitor_watch(): avoid overflow, Vladimir Sementsov-Ogievskiy, 2023/09/25
[PATCH 05/12] device_tree: qmp_dumpdtb(): stronger assertion, Vladimir Sementsov-Ogievskiy, 2023/09/25
[PATCH 06/12] mc146818rtc: rtc_set_time(): initialize tm to zeroes, Vladimir Sementsov-Ogievskiy, 2023/09/25
[PATCH 08/12] block/nvme: nvme_process_completion() fix bound for cid, Vladimir Sementsov-Ogievskiy, 2023/09/25
[PATCH 07/12] pcie_sriov: unregister_vfs(): fix error path, Vladimir Sementsov-Ogievskiy, 2023/09/25
[PATCH 10/12] hw/core/loader: gunzip(): initialize z_stream, Vladimir Sementsov-Ogievskiy, 2023/09/25
[PATCH 11/12] hw/core/loader: read_targphys(): add upper bound, Vladimir Sementsov-Ogievskiy, 2023/09/25