On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
<vsement...@yandex-team.ru> wrote:
>
> Coverity doesn't like using "untrusted" values, coming from buffers and
> fd-s as length to do IO and allocations. And that's make sense. The
> function is used three times with "untrusted" nbytes parameter. Let's
> introduce at least empirical limit of 1G for it.
>
> While being here make the function static, as it's used only here.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
> ---
> hw/core/loader.c | 13 ++++++++++---
> include/hw/loader.h | 2 --
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index aa02b27089..48cff6f59e 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -101,17 +101,24 @@ ssize_t load_image_size(const char *filename, void
> *addr, size_t size)
> return actsize < 0 ? -1 : l;
> }
>
> +#define READ_TARGPHYS_MAX_BYTES (1024 * 1024 * 1024)
> /* read()-like version */
> -ssize_t read_targphys(const char *name,
> - int fd, hwaddr dst_addr, size_t nbytes)
> +static ssize_t read_targphys(const char *name,
> + int fd, hwaddr dst_addr, size_t nbytes)
> {
> uint8_t *buf;
> ssize_t did;
>
> + if (nbytes > READ_TARGPHYS_MAX_BYTES) {
> + return -1;
> + }
> +
> buf = g_malloc(nbytes);
> did = read(fd, buf, nbytes);
> - if (did > 0)
> + if (did > 0) {
> rom_add_blob_fixed("read", buf, did, dst_addr);
> + }
> +
> g_free(buf);
> return did;
> }
This is called only from load_aout(). load_aout() is
passed a max size that it can write. So we shouldn't
be imposing a different maximum size in this utility function,
in the same way that the standard library read() does not
say "I'm going to impose a 1GB limit on how much you can read".
If the limit checks in load_aout() are wrong we should fix them.
(In any case load_aout() is used only for loading the initial
kernel on PPC and SPARC, so it's not very important.)
thanks
-- PMM
