| Message ID | 20230830203443.1126914-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user: Fixes for zero_bss | expand |
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); > } > } >
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); } }
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(-)