| Message ID | 20230925194040.68592-6-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coverity fixes | expand |
On Tue, Sep 26, 2023 at 6:42 AM Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote: > > Coverity mark this size, got from the buffer as untrasted value, it's s/untrasted/untrusted/g > 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> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > 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); > > if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) { > error_setg(errp, "Error saving FDT to file %s: %s", > -- > 2.34.1 > >
On 26.09.23 04:26, Alistair Francis wrote: > On Tue, Sep 26, 2023 at 6:42 AM Vladimir Sementsov-Ogievskiy > <vsementsov@yandex-team.ru> wrote: >> >> Coverity mark this size, got from the buffer as untrasted value, it's > > s/untrasted/untrusted/g will fix. > >> 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> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > Thanks! > >> --- >> 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); >> >> if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) { >> error_setg(errp, "Error saving FDT to file %s: %s", >> -- >> 2.34.1 >> >>
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 large size -- we'll just create a large file. The user already has lots of ways to fill up their disk if they want to, and we don't have any idea how much disk space they might or might not have. I would just mark this as a false positive. thanks -- PMM
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. In my thought clear crash on assertion is better than such memory access. > large size -- we'll just create a large file. The user already > has lots of ways to fill up their disk if they want to, and > we don't have any idea how much disk space they might or might > not have. > > I would just mark this as a false positive. > > thanks > -- PMM
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
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); if (!g_file_set_contents(filename, current_machine->fdt, size, &err)) { error_setg(errp, "Error saving FDT to file %s: %s",
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(-)