Re: [Qemu-devel] [PATCH v4 04/24] memory-device: handle integer overflows properly

[Igor Mammedov]

On Thu, 27 Sep 2018 10:13:07 +0200
David Hildenbrand <address@hidden> wrote:

> On 27/09/2018 10:02, Igor Mammedov wrote:
> > On Wed, 26 Sep 2018 11:41:59 +0200
> > David Hildenbrand <address@hidden> wrote:
> >   
> >> 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).  
> > 
> > [...]
> >   
> >> @@ -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;  
> > I'm terrible at math, lets assume size = 1 so after this
> > 'real end' would end up at 'address_space_start', which makes
> > it rather confusing beside not matching variable name anymore.  
> 
> (very simply and unrealistic) counter example as given in the
> description. You should get the idea.
> 
> address_space_start = 0xffffffffffffffffull;
that should never happen, nor valid in any conditions
just add assert so we would catch error if some patch would introduce it

> size = 1;
> 
> -> address_space_end = 0;  
> 
> While this would be perfectly valid, we would have to handle
> address_space_end potentially being 0 in the code below, because this
> would be a valid overflow. This, I avoid.
assert(address_space_end > address_space_start)
would be much better for unrealistic values without messing up with
meaning of variables.
 
> And form my POV, the variable name here matches perfectly. It points at
> the last address of the address space. (yes people have different
> opinions on this)
start,end pair is a range, there shouldn't be any other interpretations to
this variables.

> > 
> > I'd drop it and maybe extend the following assert to abort
> > on overflow condition.  
> 
> I'll leave it like this, handling address_space_end = 0 is ugly.
Looks like I have to insist on dropping this hunk.

> 
> I'll add a comment like
> 
> /* address_space_end points at the last valid address */
> 
> >   
> >>      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;  
> > I strongly suggest replace non obvious math with several
> > plain <>= conditions even if adds more conditions compared
> > to math variant.
> > At least reader doesn't have to do calculations manually
> > to figure out what's going on
> >   
> 
> Right, maybe adding a new temporary variable new_region_end will help.
what I was meaning is avoid -1 and rather use and additional range border check