Re: [PATCH 01/12] hw/core/loader: load_at(): check size

[Peter Maydell]

On Tue, 26 Sept 2023 at 11:51, Vladimir Sementsov-Ogievskiy
<vsementsov@yandex-team.ru> wrote:
>
> On 26.09.23 13:33, Peter Maydell wrote:
> > On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> > <vsementsov@yandex-team.ru> wrote:
> >>
> >> This @size parameter often comes from fd. We'd better check it before
> >> doing read and allocation.
> >>
> >> Chose 1G as high enough empiric bound.
> >
> > Empirical for who?
> >
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >> ---
> >>   hw/core/loader.c | 17 ++++++++++++++++-
> >>   1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/core/loader.c b/hw/core/loader.c
> >> index 4dd5a71fb7..4b67543046 100644
> >> --- a/hw/core/loader.c
> >> +++ b/hw/core/loader.c
> >> @@ -281,11 +281,26 @@ ssize_t load_aout(const char *filename, hwaddr addr, 
> >> int max_sz,
> >>
> >>   /* ELF loader */
> >>
> >> +#define ELF_LOAD_MAX (1024 * 1024 * 1024)
> >> +
> >>   static void *load_at(int fd, off_t offset, size_t size)
> >>   {
> >>       void *ptr;
> >> -    if (lseek(fd, offset, SEEK_SET) < 0)
> >> +
> >> +    /*
> >> +     * We often come here with @size, which was previously read from file
> >> +     * descriptor too. That's not good to read and allocate for unchecked
> >> +     * number of bytes. Coverity also doesn't like it and generate 
> >> problems.
> >> +     * So, let's limit all load_at() calls to ELF_LOAD_MAX at least.
> >> +     */
> >> +    if (size > ELF_LOAD_MAX) {
> >>           return NULL;
> >> +    }
> >> +
> >> +    if (lseek(fd, offset, SEEK_SET) < 0) {
> >> +        return NULL;
> >> +    }
> >> +
> >>       ptr = g_malloc(size);
> >>       if (read(fd, ptr, size) != size) {
> >>           g_free(ptr);
> >
> > This doesn't really help anything:
> >   (1) if the value is really big, it doesn't cause any terrible
> > consequences -- QEMU will just exit because the allocation
> > fails, which is fine because this will be at QEMU startup
> > and only happens if the user running QEMU gives us a silly file
> >   (2) we do a lot of other "allocate and abort on failure"
> > elsewhere in the ELF loader, for instance the allocations of
> > the symbol table and relocs in the load_symbols and
> > elf_reloc functions, and then on a bigger scale when we
> > work with the actual data in the ELF file
>
> Reasonable..
>
> Don't you have an idea, how to somehow mark the value "trusted" for Coverity?

In the web UI, I just mark it "false positive" in the dropdown, and
move on. Coverity has an absolute ton of false positives, and you
really can't work with it unless you have a workflow for ignoring them.

thanks
-- PMM