Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[Markus Armbruster]

David Gibson <address@hidden> writes:

> Currently get_image_size(), used to find the size of files, returns an int.
> But for modern systems, int may be only 32-bit and we can have files
> larger than that.
>
> This patch, therefore, changes the return type of get_image_size() to off_t
> (the same as the return type from lseek() itself).  It also audits all the
> callers of get_image_size() to make sure they process the new unsigned
> return type correctly.
>
> This leaves load_image_targphys() with a limited return type, but one thing
> at a time (that function has far more callers to be audited, so it will
> take longer to fix).

I'm afraid this replaces the single, well-known integer overflow in
get_image_size()'s conversion of lseek() value to int by many unknown
overflows in get_image_size()'s users.  One example below.  Didn't look
for more.

If you need a wider get_image_size(), please make sure its users are
prepared for it!

Is the any use for image sizes exceeding size_t?  Arent such images
impossible to load?

[...]
> diff --git a/hw/pc.c b/hw/pc.c
> index b9f4bc7..cb41955 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
>                         target_phys_addr_t max_ram_size)
>  {
>      uint16_t protocol;
> -    int setup_size, kernel_size, initrd_size = 0, cmdline_size;
> +    int setup_size, kernel_size, cmdline_size;
> +    off_t initrd_size = 0;
>      uint32_t initrd_max;
>      uint8_t header[8192], *setup, *kernel, *initrd_data;
>      target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
>       }
>  
>       initrd_size = get_image_size(initrd_filename);
> -        if (initrd_size < 0) {
> +        if (initrd_size == -1) {

Needless churn.

>              fprintf(stderr, "qemu: error reading initrd %s\n",
>                      initrd_filename);
>              exit(1);
           }

           initrd_addr = (initrd_max-initrd_size) & ~4095;

           initrd_data = g_malloc(initrd_size);

Integer overflow in conversion from off_t initrd_size to the argument
type size_t[*].

           load_image(initrd_filename, initrd_data);

You could try replacing load_image() by a function that returns a
pointer to the malloced image contents.

[...]


[*] Technically some glib-defined type, because they're into making up
their own "portable" types for everything.