linux-user/elfload.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
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.
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;
}
/* The first hole is before the first map entry. */
--
2.29.2
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. > > 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); do we have a test case that triggers this? > } > > /* The first hole is before the first map entry. */ -- Alex Bennée
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.
On 3/5/21 7:06 AM, Ivan A. Melnikov wrote: > Especially since, as far > as I remeber, comparing unsigned value with -1 (which is int) > is UB. It is certainly not undefined behaviour. The defined behaviour is to promote to the common type, which will do exactly what we want. r~
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 have non-zero load address.
Failing to take that 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 we are loading
(e.g. guest elf interpreter).
Downstream issue (in Russian): https://bugzilla.altlinux.org/39141
Signed-off-by: Ivan A. Melnikov <iv@altlinux.org>
---
linux-user/elfload.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index bab4237e90..58281e00f8 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2259,7 +2259,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 == (uintptr_t) -1) ? -1 : (ret - guest_loaddr);
}
/* The first hole is before the first map entry. */
--
2.29.2
Le 06/03/2021 à 10:33, Ivan A. Melnikov a écrit : > 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 have non-zero load address. > > Failing to take that 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 we are loading > (e.g. guest elf interpreter). > > Downstream issue (in Russian): https://bugzilla.altlinux.org/39141 > Signed-off-by: Ivan A. Melnikov <iv@altlinux.org> > --- > linux-user/elfload.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/linux-user/elfload.c b/linux-user/elfload.c > index bab4237e90..58281e00f8 100644 > --- a/linux-user/elfload.c > +++ b/linux-user/elfload.c > @@ -2259,7 +2259,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 == (uintptr_t) -1) ? -1 : (ret - guest_loaddr); You don't want the uintptr_t as ret is intptr_t > } > > /* The first hole is before the first map entry. */ > I've already added the patch from Vincent Fazio to fix the problem. https://github.com/vivier/qemu/commit/5112f50d6e889a2a1098fb2a67a0851641c350f4 It will be in my next pull request. Thanks, Laurent
© 2016 - 2024 Red Hat, Inc.