Re: [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr

[Ivan A. Melnikov]

Alex, thank you for the review.

On Fri, Mar 05, 2021 at 02:03:43PM +0000, Alex Bennée wrote:
> 
> Ivan A. Melnikov <iv@altlinux.org> writes:
> 
> > While pgd_find_hole_fallback returns the beginning of the
> > hole found, pgb_find_hole returns guest_base, which
> > is somewhat different as the binary qemu-user is loading
> > usually has non-zero load address.
> >
> > Failing to take this into account leads to random crashes
> > if the hole is "just big enough", but not bigger:
> > in that case, important mappings (e.g. parts of qemu-user
> > itself) may be replaced with the binary it is loading
> > (e.g. the guest elf interpreter).
> >
> > This patch also fixes the return type of pgd_find_hole_fallback:
> > it returns -1 if no hole is found, so a signed return type
> > should be used.
> 
> I don't think it should. For one thing the type is preserved as
> uintptr_t all the way up the call chain so just changing it here doesn't
> help much. -1 is really just a quick way of saying all bits are set
> which is the one "fail" value we check for. The address space is big
> enough we could theoretically return a chunk of space that otherwise has
> the top bit set.

I see your point. I'd only suggest to be explicit about using all-ones
as a spectial value, something like this:

static const uintptr_t invalid_poitner = ~0ULL;

and then using it as a return value. Especially since, as far
as I remeber, comparing unsigned value with -1 (which is int)
is UB.

Makes sense?

> > Downstream issue (in Russian): https://bugzilla.altlinux.org/39141
> > Signed-off-by: Ivan A. Melnikov <iv@altlinux.org>
> > ---
> >  linux-user/elfload.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> > index bab4237e90..acd510532c 100644
> > --- a/linux-user/elfload.c
> > +++ b/linux-user/elfload.c
> > @@ -2205,9 +2205,11 @@ static void pgb_have_guest_base(const char 
> > *image_name, abi_ulong guest_loaddr,
> >   * /proc/self/map. It can potentially take a very long time as we can
> >   * only dumbly iterate up the host address space seeing if the
> >   * allocation would work.
> > + *
> > + * Returns the start addres of the hole found, or -1 if no hole found.
> >   */
> > -static uintptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t 
> > brk,
> > -                                        long align, uintptr_t offset)
> > +static intptr_t pgd_find_hole_fallback(uintptr_t guest_size, uintptr_t brk,
> > +                                       long align, uintptr_t offset)
> >  {
> >      uintptr_t base;
> >  
> > @@ -2235,7 +2237,7 @@ static uintptr_t pgd_find_hole_fallback(uintptr_t 
> > guest_size, uintptr_t brk,
> >                  munmap((void *) align_start, guest_size);
> >                  if (MAP_FIXED_NOREPLACE != 0 ||
> >                      mmap_start == (void *) align_start) {
> > -                    return (uintptr_t) mmap_start + offset;
> > +                    return (intptr_t) mmap_start + offset;
> >                  }
> >              }
> >              base += qemu_host_page_size;
> > @@ -2259,7 +2261,8 @@ static uintptr_t pgb_find_hole(uintptr_t 
> > guest_loaddr, uintptr_t guest_size,
> >      brk = (uintptr_t)sbrk(0);
> >  
> >      if (!maps) {
> > -        return pgd_find_hole_fallback(guest_size, brk, align, offset);
> > +        ret = pgd_find_hole_fallback(guest_size, brk, align, offset);
> > +        return (ret > guest_loaddr) ? (ret - guest_loaddr) : -1;
> 
> So I think we just want:
> 
>     return ret == -1 ? -1 : (ret - guest_loaddr);

This will work for me as well. I'm just a bit hesitant with putting
in some kind of integer underflow when it seems easily avoidable;
but if we stick with uintptr_t that should non matter.

> do we have a test case that triggers this?

I don't think there are test cases that cover the 
pgd_find_hole_fallback code path, at least when
the test suite is run on a system with /proc mounted.

-- 
  wbr,
    iv m.