* [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr
@ 2021-03-03 9:49 Ivan A. Melnikov
2021-03-05 14:03 ` Alex Bennée
2021-03-06 9:33 ` [PATCH v2] " Ivan A. Melnikov
0 siblings, 2 replies; 6+ messages in thread
From: Ivan A. Melnikov @ 2021-03-03 9:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Bennée, Laurent Vivier
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr
2021-03-03 9:49 [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr Ivan A. Melnikov
@ 2021-03-05 14:03 ` Alex Bennée
2021-03-05 15:06 ` Ivan A. Melnikov
2021-03-06 9:33 ` [PATCH v2] " Ivan A. Melnikov
1 sibling, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2021-03-05 14:03 UTC (permalink / raw)
To: Ivan A. Melnikov; +Cc: qemu-devel, Laurent Vivier
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr
2021-03-05 14:03 ` Alex Bennée
@ 2021-03-05 15:06 ` Ivan A. Melnikov
2021-03-05 15:39 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Ivan A. Melnikov @ 2021-03-05 15:06 UTC (permalink / raw)
To: Alex Bennée; +Cc: qemu-devel, Laurent Vivier
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr
2021-03-05 15:06 ` Ivan A. Melnikov
@ 2021-03-05 15:39 ` Richard Henderson
0 siblings, 0 replies; 6+ messages in thread
From: Richard Henderson @ 2021-03-05 15:39 UTC (permalink / raw)
To: Ivan A. Melnikov, Alex Bennée; +Cc: qemu-devel, Laurent Vivier
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~
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr
2021-03-03 9:49 [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr Ivan A. Melnikov
2021-03-05 14:03 ` Alex Bennée
@ 2021-03-06 9:33 ` Ivan A. Melnikov
2021-03-09 21:49 ` Laurent Vivier
1 sibling, 1 reply; 6+ messages in thread
From: Ivan A. Melnikov @ 2021-03-06 9:33 UTC (permalink / raw)
To: qemu-devel; +Cc: Alex Bennée, Laurent Vivier
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr
2021-03-06 9:33 ` [PATCH v2] " Ivan A. Melnikov
@ 2021-03-09 21:49 ` Laurent Vivier
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Vivier @ 2021-03-09 21:49 UTC (permalink / raw)
To: Ivan A. Melnikov, qemu-devel; +Cc: Alex Bennée, Vincent Fazio
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-09 21:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 9:49 [PATCH] linux-user: Adjust pgd_find_hole_fallback result with guest_loaddr Ivan A. Melnikov
2021-03-05 14:03 ` Alex Bennée
2021-03-05 15:06 ` Ivan A. Melnikov
2021-03-05 15:39 ` Richard Henderson
2021-03-06 9:33 ` [PATCH v2] " Ivan A. Melnikov
2021-03-09 21:49 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.