Re: [Qemu-ppc] [PATCH v3 02/22] memory-device: handle integer overflows properly

[David Gibson]

On Thu, Sep 20, 2018 at 12:32:23PM +0200, David Hildenbrand wrote:
> Although unlikely in practice, we could have integer overflows on some
> calculations based on addresses and sizes, leading to error checks not
> triggering.
> 
> Let's properly handle this whenever we do an addition. Make
> address_space_end point at the real end, instead of end + 1, so we don't
> have to handle special cases like it being 0. This will allow us to
> place a memory device at the very end of the guest physical 64bit address
> space (if ever possible).
> 
> Also, QEMU_ALIGN_UP(md_addr + md_size, align) could (theoretically) wrap to
> address 0, so add a final check for 0.
> 
> Reported-by: Dr. David Alan Gilbert <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>

Reviewed-by: David Gibson <address@hidden>

> ---
>  hw/mem/memory-device.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index efacbc2a7d..020aad102a 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -85,7 +85,8 @@ static void memory_device_check_addable(MachineState *ms, 
> uint64_t size,
>  
>      /* will we exceed the total amount of memory specified */
>      memory_device_used_region_size(OBJECT(ms), &used_region_size);
> -    if (used_region_size + size > ms->maxram_size - ms->ram_size) {
> +    if (used_region_size + size < used_region_size ||
> +        used_region_size + size > ms->maxram_size - ms->ram_size) {
>          error_setg(errp, "not enough space, currently 0x%" PRIx64
>                     " in use of total hot pluggable 0x" RAM_ADDR_FMT,
>                     used_region_size, ms->maxram_size - ms->ram_size);
> @@ -115,7 +116,7 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>      }
>      address_space_start = ms->device_memory->base;
>      address_space_end = address_space_start +
> -                        memory_region_size(&ms->device_memory->mr);
> +                        memory_region_size(&ms->device_memory->mr) - 1;
>      g_assert(address_space_end >= address_space_start);
>  
>      /* address_space_start indicates the maximum alignment we expect */
> @@ -149,7 +150,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>                         "] before 0x%" PRIx64, new_addr, size,
>                         address_space_start);
>              return 0;
> -        } else if ((new_addr + size) > address_space_end) {
> +        } else if (new_addr + size - 1 < new_addr ||
> +                   new_addr + size - 1 > address_space_end) {
>              error_setg(errp, "can't add memory [0x%" PRIx64 ":0x%" PRIx64
>                         "] beyond 0x%" PRIx64, new_addr, size,
>                         address_space_end);
> @@ -182,7 +184,8 @@ uint64_t memory_device_get_free_addr(MachineState *ms, 
> const uint64_t *hint,
>          }
>      }
>  
> -    if (new_addr + size > address_space_end) {
> +    if (new_addr + size - 1 < new_addr || !new_addr ||
> +        new_addr + size - 1 > address_space_end) {
>          error_setg(errp, "could not find position in guest address space for 
> "
>                     "memory device - memory fragmented due to alignments");
>          goto out;

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature