[PATCH] linux-user: Fixes for zero_bss

[PATCH] linux-user: Fixes for zero_bss

[Richard Henderson]

The previous change, 2d385be6152, assumed !PAGE_VALID meant that
the page would be unmapped by the elf image.  However, since we
reserved the entire image space via mmap, PAGE_VALID will always
be set.  Instead, assume PROT_NONE for the same condition.

Furthermore, assume bss is only ever present for writable segments,
and that there is no page overlap between PT_LOAD segments.
Instead of an assert, return false to indicate failure.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1854
Fixes: 2d385be6152 ("linux-user: Do not adjust zero_bss for host page size")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/elfload.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index a5b28fa3e7..7bc7bcec19 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2308,21 +2308,40 @@ static bool zero_bss(abi_ulong start_bss, abi_ulong end_bss, int prot)
 {
     abi_ulong align_bss;
 
+    /* We only expect writable bss; the code segment shouldn't need this. */
+    if (!(prot & PROT_WRITE)) {
+        return false;
+    }
+
     align_bss = TARGET_PAGE_ALIGN(start_bss);
     end_bss = TARGET_PAGE_ALIGN(end_bss);
 
     if (start_bss < align_bss) {
         int flags = page_get_flags(start_bss);
 
-        if (!(flags & PAGE_VALID)) {
-            /* Map the start of the bss. */
+        if (!(flags & PAGE_BITS)) {
+            /*
+             * The whole address space of the executable was reserved
+             * at the start, therefore all pages will be VALID.
+             * But assuming there are no PROT_NONE PT_LOAD segments,
+             * a PROT_NONE page means no data all bss, and we can
+             * simply extend the new anon mapping back to the start
+             * of the page of bss.
+             */
             align_bss -= TARGET_PAGE_SIZE;
-        } else if (flags & PAGE_WRITE) {
-            /* The page is already mapped writable. */
-            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
         } else {
-            /* Read-only zeros? */
-            g_assert_not_reached();
+            /*
+             * The start of the bss shares a page with something.
+             * The only thing that we expect is the data section,
+             * which would already be marked writable.
+             * Overlapping the RX code segment seems malformed.
+             */
+            if (!(flags & PAGE_WRITE)) {
+                return false;
+            }
+
+            /* The page is already mapped and writable. */
+            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
         }
     }
 
-- 
2.34.1

Re: [PATCH] linux-user: Fixes for zero_bss

[Philippe Mathieu-Daudé]

On 30/8/23 22:34, Richard Henderson wrote:
> The previous change, 2d385be6152, assumed !PAGE_VALID meant that
> the page would be unmapped by the elf image.  However, since we
> reserved the entire image space via mmap, PAGE_VALID will always
> be set.  Instead, assume PROT_NONE for the same condition.
> 
> Furthermore, assume bss is only ever present for writable segments,
> and that there is no page overlap between PT_LOAD segments.
> Instead of an assert, return false to indicate failure.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1854
> Fixes: 2d385be6152 ("linux-user: Do not adjust zero_bss for host page size")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/elfload.c | 33 ++++++++++++++++++++++++++-------
>   1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index a5b28fa3e7..7bc7bcec19 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2308,21 +2308,40 @@ static bool zero_bss(abi_ulong start_bss, abi_ulong end_bss, int prot)
>   {
>       abi_ulong align_bss;
>   
> +    /* We only expect writable bss; the code segment shouldn't need this. */
> +    if (!(prot & PROT_WRITE)) {
> +        return false;

Caller will fail with random errno and "Error mapping file".
Not really accurate. Maybe we could pass an Error* paramenter
to zero_bss().

> +    }
> +
>       align_bss = TARGET_PAGE_ALIGN(start_bss);
>       end_bss = TARGET_PAGE_ALIGN(end_bss);
>   
>       if (start_bss < align_bss) {
>           int flags = page_get_flags(start_bss);
>   
> -        if (!(flags & PAGE_VALID)) {
> -            /* Map the start of the bss. */
> +        if (!(flags & PAGE_BITS)) {
> +            /*
> +             * The whole address space of the executable was reserved
> +             * at the start, therefore all pages will be VALID.
> +             * But assuming there are no PROT_NONE PT_LOAD segments,
> +             * a PROT_NONE page means no data all bss, and we can
> +             * simply extend the new anon mapping back to the start
> +             * of the page of bss.
> +             */
>               align_bss -= TARGET_PAGE_SIZE;
> -        } else if (flags & PAGE_WRITE) {
> -            /* The page is already mapped writable. */
> -            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
>           } else {
> -            /* Read-only zeros? */
> -            g_assert_not_reached();
> +            /*
> +             * The start of the bss shares a page with something.
> +             * The only thing that we expect is the data section,
> +             * which would already be marked writable.
> +             * Overlapping the RX code segment seems malformed.
> +             */
> +            if (!(flags & PAGE_WRITE)) {

Similar random errno and "Error mapping file".

> +                return false;
> +            }
> +
> +            /* The page is already mapped and writable. */
> +            memset(g2h_untagged(start_bss), 0, align_bss - start_bss);
>           }
>       }
>