* [PATCH] linux-user: Fix loading of BSS segments
@ 2020-12-17 10:17 Giuseppe Musacchio
2020-12-17 10:48 ` Laurent Vivier
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Giuseppe Musacchio @ 2020-12-17 10:17 UTC (permalink / raw)
To: QEMU Developers; +Cc: Peter Maydell, Richard Henderson, Laurent Vivier
Some ELF binaries encode the .bss section as an extension of the data
ones by setting the segment p_memsz > p_filesz. Some other binaries take
a different route and encode it as a stand-alone PT_LOAD segment with
p_filesz = 0 and p_memsz > 0.
Both the encodings are actually correct per ELF specification but the
ELF loader had some troubles in handling the former: with the old logic
it was very likely to get Qemu to crash in zero_bss when trying to
access unmapped memory.
zero_bss isn't meant to allocate whole zero-filled segments but to
"complete" a previously mapped segment with the needed zero bits.
The fix is pretty simple, if the segment is completely zero-filled we
simply allocate one or more pages (according to p_memsz) and avoid
calling zero_bss altogether.
Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
---
linux-user/elfload.c | 30 ++++++++++++++++++++----------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 0b02a92602..a16c240e0f 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2776,14 +2776,16 @@ static void load_elf_image(const char *image_name, int image_fd,
vaddr = load_bias + eppnt->p_vaddr;
vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
- vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
+
+ vaddr_ef = vaddr + eppnt->p_filesz;
+ vaddr_em = vaddr + eppnt->p_memsz;
/*
- * Some segments may be completely empty without any backing file
- * segment, in that case just let zero_bss allocate an empty buffer
- * for it.
+ * Some segments may be completely empty, with a non-zero p_memsz
+ * but no backing file segment.
*/
if (eppnt->p_filesz != 0) {
+ vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
MAP_PRIVATE | MAP_FIXED,
image_fd, eppnt->p_offset - vaddr_po);
@@ -2791,14 +2793,22 @@ static void load_elf_image(const char *image_name, int image_fd,
if (error == -1) {
goto exit_mmap;
}
- }
- vaddr_ef = vaddr + eppnt->p_filesz;
- vaddr_em = vaddr + eppnt->p_memsz;
+ /*
+ * If the load segment requests extra zeros (e.g. bss), map it.
+ */
+ if (eppnt->p_filesz < eppnt->p_memsz) {
+ zero_bss(vaddr_ef, vaddr_em, elf_prot);
+ }
+ } else if (eppnt->p_memsz != 0) {
+ vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_memsz + vaddr_po);
+ error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
+ MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
+ -1, 0);
- /* If the load segment requests extra zeros (e.g. bss), map it. */
- if (vaddr_ef < vaddr_em) {
- zero_bss(vaddr_ef, vaddr_em, elf_prot);
+ if (error == -1) {
+ goto exit_mmap;
+ }
}
/* Find the full program boundaries. */
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-17 10:17 [PATCH] linux-user: Fix loading of BSS segments Giuseppe Musacchio
@ 2020-12-17 10:48 ` Laurent Vivier
2020-12-17 10:59 ` Giuseppe Musacchio
2020-12-17 17:40 ` Stephen Long
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2020-12-17 10:48 UTC (permalink / raw)
To: Giuseppe Musacchio, QEMU Developers
Cc: Peter Maydell, Ben Hutchings, Richard Henderson,
philippe.mathieu.daude, Stephen Long
Le 17/12/2020 à 11:17, Giuseppe Musacchio a écrit :
> Some ELF binaries encode the .bss section as an extension of the data
> ones by setting the segment p_memsz > p_filesz. Some other binaries take
> a different route and encode it as a stand-alone PT_LOAD segment with
> p_filesz = 0 and p_memsz > 0.
>
> Both the encodings are actually correct per ELF specification but the
> ELF loader had some troubles in handling the former: with the old logic
> it was very likely to get Qemu to crash in zero_bss when trying to
> access unmapped memory.
>
> zero_bss isn't meant to allocate whole zero-filled segments but to
> "complete" a previously mapped segment with the needed zero bits.
>
> The fix is pretty simple, if the segment is completely zero-filled we
> simply allocate one or more pages (according to p_memsz) and avoid
> calling zero_bss altogether.
Is this also fixing what "linux-user/elfload: Fix handling of pure BSS segments" [1] patch fixes?
Thanks,
Laurent
[1] https://patchew.org/QEMU/20201118165206.2826-1-steplong@quicinc.com/
> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
> ---
> linux-user/elfload.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 0b02a92602..a16c240e0f 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2776,14 +2776,16 @@ static void load_elf_image(const char *image_name, int image_fd,
> vaddr = load_bias + eppnt->p_vaddr;
> vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
> vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
> - vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
> +
> + vaddr_ef = vaddr + eppnt->p_filesz;
> + vaddr_em = vaddr + eppnt->p_memsz;
>
> /*
> - * Some segments may be completely empty without any backing file
> - * segment, in that case just let zero_bss allocate an empty buffer
> - * for it.
> + * Some segments may be completely empty, with a non-zero p_memsz
> + * but no backing file segment.
> */
> if (eppnt->p_filesz != 0) {
> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
> error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> MAP_PRIVATE | MAP_FIXED,
> image_fd, eppnt->p_offset - vaddr_po);
> @@ -2791,14 +2793,22 @@ static void load_elf_image(const char *image_name, int image_fd,
> if (error == -1) {
> goto exit_mmap;
> }
> - }
>
> - vaddr_ef = vaddr + eppnt->p_filesz;
> - vaddr_em = vaddr + eppnt->p_memsz;
> + /*
> + * If the load segment requests extra zeros (e.g. bss), map it.
> + */
> + if (eppnt->p_filesz < eppnt->p_memsz) {
> + zero_bss(vaddr_ef, vaddr_em, elf_prot);
> + }
> + } else if (eppnt->p_memsz != 0) {
> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_memsz + vaddr_po);
> + error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> + MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
> + -1, 0);
>
> - /* If the load segment requests extra zeros (e.g. bss), map it. */
> - if (vaddr_ef < vaddr_em) {
> - zero_bss(vaddr_ef, vaddr_em, elf_prot);
> + if (error == -1) {
> + goto exit_mmap;
> + }
> }
>
> /* Find the full program boundaries. */
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-17 10:48 ` Laurent Vivier
@ 2020-12-17 10:59 ` Giuseppe Musacchio
0 siblings, 0 replies; 9+ messages in thread
From: Giuseppe Musacchio @ 2020-12-17 10:59 UTC (permalink / raw)
To: Laurent Vivier, QEMU Developers
Cc: Peter Maydell, Ben Hutchings, Richard Henderson,
philippe.mathieu.daude, Stephen Long
On 17/12/20 11:48, Laurent Vivier wrote:
> Le 17/12/2020 à 11:17, Giuseppe Musacchio a écrit :
>> Some ELF binaries encode the .bss section as an extension of the data
>> ones by setting the segment p_memsz > p_filesz. Some other binaries take
>> a different route and encode it as a stand-alone PT_LOAD segment with
>> p_filesz = 0 and p_memsz > 0.
>>
>> Both the encodings are actually correct per ELF specification but the
>> ELF loader had some troubles in handling the former: with the old logic
>> it was very likely to get Qemu to crash in zero_bss when trying to
>> access unmapped memory.
>>
>> zero_bss isn't meant to allocate whole zero-filled segments but to
>> "complete" a previously mapped segment with the needed zero bits.
>>
>> The fix is pretty simple, if the segment is completely zero-filled we
>> simply allocate one or more pages (according to p_memsz) and avoid
>> calling zero_bss altogether.
>
>
> Is this also fixing what "linux-user/elfload: Fix handling of pure BSS segments" [1] patch fixes?
>
Yes, I guess I'm not the first one to hit this problem.
> Thanks,
> Laurent
> [1] https://patchew.org/QEMU/20201118165206.2826-1-steplong@quicinc.com/
>
>> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
>> ---
>> linux-user/elfload.c | 30 ++++++++++++++++++++----------
>> 1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 0b02a92602..a16c240e0f 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -2776,14 +2776,16 @@ static void load_elf_image(const char *image_name, int image_fd,
>> vaddr = load_bias + eppnt->p_vaddr;
>> vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
>> vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
>> - vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
>> +
>> + vaddr_ef = vaddr + eppnt->p_filesz;
>> + vaddr_em = vaddr + eppnt->p_memsz;
>>
>> /*
>> - * Some segments may be completely empty without any backing file
>> - * segment, in that case just let zero_bss allocate an empty buffer
>> - * for it.
>> + * Some segments may be completely empty, with a non-zero p_memsz
>> + * but no backing file segment.
>> */
>> if (eppnt->p_filesz != 0) {
>> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
>> error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
>> MAP_PRIVATE | MAP_FIXED,
>> image_fd, eppnt->p_offset - vaddr_po);
>> @@ -2791,14 +2793,22 @@ static void load_elf_image(const char *image_name, int image_fd,
>> if (error == -1) {
>> goto exit_mmap;
>> }
>> - }
>>
>> - vaddr_ef = vaddr + eppnt->p_filesz;
>> - vaddr_em = vaddr + eppnt->p_memsz;
>> + /*
>> + * If the load segment requests extra zeros (e.g. bss), map it.
>> + */
>> + if (eppnt->p_filesz < eppnt->p_memsz) {
>> + zero_bss(vaddr_ef, vaddr_em, elf_prot);
>> + }
>> + } else if (eppnt->p_memsz != 0) {
>> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_memsz + vaddr_po);
>> + error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
>> + MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
>> + -1, 0);
>>
>> - /* If the load segment requests extra zeros (e.g. bss), map it. */
>> - if (vaddr_ef < vaddr_em) {
>> - zero_bss(vaddr_ef, vaddr_em, elf_prot);
>> + if (error == -1) {
>> + goto exit_mmap;
>> + }
>> }
>>
>> /* Find the full program boundaries. */
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-17 10:17 [PATCH] linux-user: Fix loading of BSS segments Giuseppe Musacchio
2020-12-17 10:48 ` Laurent Vivier
@ 2020-12-17 17:40 ` Stephen Long
2020-12-17 20:07 ` Laurent Vivier
2020-12-17 20:27 ` Stephen Long
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Stephen Long @ 2020-12-17 17:40 UTC (permalink / raw)
To: laurent, qemu-devel
Cc: thatlemon, philippe.mathieu.daude, richard.henderson, peter.maydell, ben
Laurent Vivier wrote:
> Is this also fixing what "linux-user/elfload: Fix handling of pure BSS segments" [1] patch fixes?
I can do a v2 of my patch with a better commit description and addressing Peter's
questions, but feel free to take this patch instead. It has a much
clearer commit msg and seems to be more correct to me.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-17 17:40 ` Stephen Long
@ 2020-12-17 20:07 ` Laurent Vivier
0 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2020-12-17 20:07 UTC (permalink / raw)
To: Stephen Long, qemu-devel
Cc: thatlemon, richard.henderson, ben, philippe.mathieu.daude, peter.maydell
Le 17/12/2020 à 18:40, Stephen Long a écrit :
> Laurent Vivier wrote:
>> Is this also fixing what "linux-user/elfload: Fix handling of pure BSS segments" [1] patch fixes?
>
> I can do a v2 of my patch with a better commit description and addressing Peter's
> questions, but feel free to take this patch instead. It has a much
> clearer commit msg and seems to be more correct to me.
>
If this patch fixes also your problem I will take this one instead.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-17 10:17 [PATCH] linux-user: Fix loading of BSS segments Giuseppe Musacchio
2020-12-17 10:48 ` Laurent Vivier
2020-12-17 17:40 ` Stephen Long
@ 2020-12-17 20:27 ` Stephen Long
2020-12-19 10:55 ` Laurent Vivier
2021-02-13 16:41 ` Laurent Vivier
4 siblings, 0 replies; 9+ messages in thread
From: Stephen Long @ 2020-12-17 20:27 UTC (permalink / raw)
To: laurent, qemu-devel
Cc: thatlemon, philippe.mathieu.daude, richard.henderson, peter.maydell, ben
Laurent Vivier wrote:
> If this patch fixes also your problem I will take this one instead.
Awesome, I can confirm that this patch fixes my problem.
Thanks,
Stephen
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-17 10:17 [PATCH] linux-user: Fix loading of BSS segments Giuseppe Musacchio
` (2 preceding siblings ...)
2020-12-17 20:27 ` Stephen Long
@ 2020-12-19 10:55 ` Laurent Vivier
2020-12-19 17:46 ` Giuseppe Musacchio
2021-02-13 16:41 ` Laurent Vivier
4 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2020-12-19 10:55 UTC (permalink / raw)
To: Giuseppe Musacchio, QEMU Developers; +Cc: Peter Maydell, Richard Henderson
Le 17/12/2020 à 11:17, Giuseppe Musacchio a écrit :
> Some ELF binaries encode the .bss section as an extension of the data
> ones by setting the segment p_memsz > p_filesz. Some other binaries take
> a different route and encode it as a stand-alone PT_LOAD segment with
> p_filesz = 0 and p_memsz > 0.
>
> Both the encodings are actually correct per ELF specification but the
> ELF loader had some troubles in handling the former: with the old logic
> it was very likely to get Qemu to crash in zero_bss when trying to
> access unmapped memory.
>
> zero_bss isn't meant to allocate whole zero-filled segments but to
> "complete" a previously mapped segment with the needed zero bits.
>
> The fix is pretty simple, if the segment is completely zero-filled we
> simply allocate one or more pages (according to p_memsz) and avoid
> calling zero_bss altogether.
So, the current code manages the bss segment when the memory page has already
been allocated for the data segment by zeroing it:
+----------------------------------+
PAGE |
----------+------------+ |
DATA | BSS | |
So your patch fixes the case when there is no data segment and thus no page
to complete:
+----------------------------------+
PAGE |
----------+ |
BSS | |
But could we have a case where the BSS starts in a page allocated for the
data segment but needs more pages?
+----------------------------------+----------------------------------+
PAGE | PAGE |
------------------------+----------------------------+ |
DATA | BSS | |
In this case we should also allocate the page, and the previous case is only a
special case (data segment = 0) of this one.
so something like (approxymately):
if (eppnt->p_filesz != 0) {
target_mmap()
if (vaddr_ef < vaddr_mem) {
zero_bss(vaddr_ef, MIN(vaddr_mem, vaddr_ps + vaddr_len))
}
}
if (vaddr_ps + vaddr_len < vaddr_mem) {
target_mmap(vaddr_ps + vaddr_len, vaddr_ps + vaddr_len - vaddr_mem - 1,
elf_prot, MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
}
I think your fix is correct, but I'm wondering if we can have something more
generic, if we can cover an other possible case.
If you think we don't need to introduce more complexity for a case that can't
happen I will queue your patch as is.
Thanks,
Laurent
> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
> ---
> linux-user/elfload.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 0b02a92602..a16c240e0f 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2776,14 +2776,16 @@ static void load_elf_image(const char *image_name, int image_fd,
> vaddr = load_bias + eppnt->p_vaddr;
> vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
> vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
> - vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
> +
> + vaddr_ef = vaddr + eppnt->p_filesz;
> + vaddr_em = vaddr + eppnt->p_memsz;
>
> /*
> - * Some segments may be completely empty without any backing file
> - * segment, in that case just let zero_bss allocate an empty buffer
> - * for it.
> + * Some segments may be completely empty, with a non-zero p_memsz
> + * but no backing file segment.
> */
> if (eppnt->p_filesz != 0) {
> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
> error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> MAP_PRIVATE | MAP_FIXED,
> image_fd, eppnt->p_offset - vaddr_po);
> @@ -2791,14 +2793,22 @@ static void load_elf_image(const char *image_name, int image_fd,
> if (error == -1) {
> goto exit_mmap;
> }
> - }
>
> - vaddr_ef = vaddr + eppnt->p_filesz;
> - vaddr_em = vaddr + eppnt->p_memsz;
> + /*
> + * If the load segment requests extra zeros (e.g. bss), map it.
> + */
> + if (eppnt->p_filesz < eppnt->p_memsz) {
> + zero_bss(vaddr_ef, vaddr_em, elf_prot);
> + }
> + } else if (eppnt->p_memsz != 0) {
> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_memsz + vaddr_po);
> + error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> + MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
> + -1, 0);
>
> - /* If the load segment requests extra zeros (e.g. bss), map it. */
> - if (vaddr_ef < vaddr_em) {
> - zero_bss(vaddr_ef, vaddr_em, elf_prot);
> + if (error == -1) {
> + goto exit_mmap;
> + }
> }
>
> /* Find the full program boundaries. */
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-19 10:55 ` Laurent Vivier
@ 2020-12-19 17:46 ` Giuseppe Musacchio
0 siblings, 0 replies; 9+ messages in thread
From: Giuseppe Musacchio @ 2020-12-19 17:46 UTC (permalink / raw)
To: Laurent Vivier, QEMU Developers; +Cc: Peter Maydell, Richard Henderson
On 19/12/20 11:55, Laurent Vivier wrote:
> Le 17/12/2020 à 11:17, Giuseppe Musacchio a écrit :
>> Some ELF binaries encode the .bss section as an extension of the data
>> ones by setting the segment p_memsz > p_filesz. Some other binaries take
>> a different route and encode it as a stand-alone PT_LOAD segment with
>> p_filesz = 0 and p_memsz > 0.
>>
>> Both the encodings are actually correct per ELF specification but the
>> ELF loader had some troubles in handling the former: with the old logic
>> it was very likely to get Qemu to crash in zero_bss when trying to
>> access unmapped memory.
>>
>> zero_bss isn't meant to allocate whole zero-filled segments but to
>> "complete" a previously mapped segment with the needed zero bits.
>>
>> The fix is pretty simple, if the segment is completely zero-filled we
>> simply allocate one or more pages (according to p_memsz) and avoid
>> calling zero_bss altogether.
>
> So, the current code manages the bss segment when the memory page has already
> been allocated for the data segment by zeroing it:
>
> +----------------------------------+
> PAGE |
> ----------+------------+ |
> DATA | BSS | |
>
> So your patch fixes the case when there is no data segment and thus no page
> to complete:
>
> +----------------------------------+
> PAGE |
> ----------+ |
> BSS | |
>
>
> But could we have a case where the BSS starts in a page allocated for the
> data segment but needs more pages?
>
> +----------------------------------+----------------------------------+
> PAGE | PAGE |
> ------------------------+----------------------------+ |
> DATA | BSS | |
>
> In this case we should also allocate the page, and the previous case is only a
> special case (data segment = 0) of this one.
>
If I correctly understand your example here this case should be already
handled by zero_bss: the first chunk of .bss is mapped as part of the
vaddr_len mapping and is zeroed by the memset, while the other chunk/page
is mapped in the `host_map_start < host_end` clause.
> so something like (approxymately):
>
> if (eppnt->p_filesz != 0) {
> target_mmap()
> if (vaddr_ef < vaddr_mem) {
> zero_bss(vaddr_ef, MIN(vaddr_mem, vaddr_ps + vaddr_len))
> }
> }
> if (vaddr_ps + vaddr_len < vaddr_mem) {
> target_mmap(vaddr_ps + vaddr_len, vaddr_ps + vaddr_len - vaddr_mem - 1,
> elf_prot, MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
> }
>
> I think your fix is correct, but I'm wondering if we can have something more
> generic, if we can cover an other possible case.
>
> If you think we don't need to introduce more complexity for a case that can't
> happen I will queue your patch as is.
>
> Thanks,
> Laurent
>
>> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
>> ---
>> linux-user/elfload.c | 30 ++++++++++++++++++++----------
>> 1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index 0b02a92602..a16c240e0f 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -2776,14 +2776,16 @@ static void load_elf_image(const char *image_name, int image_fd,
>> vaddr = load_bias + eppnt->p_vaddr;
>> vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
>> vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
>> - vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
>> +
>> + vaddr_ef = vaddr + eppnt->p_filesz;
>> + vaddr_em = vaddr + eppnt->p_memsz;
>>
>> /*
>> - * Some segments may be completely empty without any backing file
>> - * segment, in that case just let zero_bss allocate an empty buffer
>> - * for it.
>> + * Some segments may be completely empty, with a non-zero p_memsz
>> + * but no backing file segment.
>> */
>> if (eppnt->p_filesz != 0) {
>> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
>> error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
>> MAP_PRIVATE | MAP_FIXED,
>> image_fd, eppnt->p_offset - vaddr_po);
>> @@ -2791,14 +2793,22 @@ static void load_elf_image(const char *image_name, int image_fd,
>> if (error == -1) {
>> goto exit_mmap;
>> }
>> - }
>>
>> - vaddr_ef = vaddr + eppnt->p_filesz;
>> - vaddr_em = vaddr + eppnt->p_memsz;
>> + /*
>> + * If the load segment requests extra zeros (e.g. bss), map it.
>> + */
>> + if (eppnt->p_filesz < eppnt->p_memsz) {
>> + zero_bss(vaddr_ef, vaddr_em, elf_prot);
>> + }
>> + } else if (eppnt->p_memsz != 0) {
>> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_memsz + vaddr_po);
>> + error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
>> + MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
>> + -1, 0);
>>
>> - /* If the load segment requests extra zeros (e.g. bss), map it. */
>> - if (vaddr_ef < vaddr_em) {
>> - zero_bss(vaddr_ef, vaddr_em, elf_prot);
>> + if (error == -1) {
>> + goto exit_mmap;
>> + }
>> }
>>
>> /* Find the full program boundaries. */
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] linux-user: Fix loading of BSS segments
2020-12-17 10:17 [PATCH] linux-user: Fix loading of BSS segments Giuseppe Musacchio
` (3 preceding siblings ...)
2020-12-19 10:55 ` Laurent Vivier
@ 2021-02-13 16:41 ` Laurent Vivier
4 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2021-02-13 16:41 UTC (permalink / raw)
To: Giuseppe Musacchio, QEMU Developers; +Cc: Peter Maydell, Richard Henderson
Le 17/12/2020 à 11:17, Giuseppe Musacchio a écrit :
> Some ELF binaries encode the .bss section as an extension of the data
> ones by setting the segment p_memsz > p_filesz. Some other binaries take
> a different route and encode it as a stand-alone PT_LOAD segment with
> p_filesz = 0 and p_memsz > 0.
>
> Both the encodings are actually correct per ELF specification but the
> ELF loader had some troubles in handling the former: with the old logic
> it was very likely to get Qemu to crash in zero_bss when trying to
> access unmapped memory.
>
> zero_bss isn't meant to allocate whole zero-filled segments but to
> "complete" a previously mapped segment with the needed zero bits.
>
> The fix is pretty simple, if the segment is completely zero-filled we
> simply allocate one or more pages (according to p_memsz) and avoid
> calling zero_bss altogether.
>
> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
> ---
> linux-user/elfload.c | 30 ++++++++++++++++++++----------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 0b02a92602..a16c240e0f 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2776,14 +2776,16 @@ static void load_elf_image(const char *image_name, int image_fd,
> vaddr = load_bias + eppnt->p_vaddr;
> vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
> vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
> - vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
> +
> + vaddr_ef = vaddr + eppnt->p_filesz;
> + vaddr_em = vaddr + eppnt->p_memsz;
>
> /*
> - * Some segments may be completely empty without any backing file
> - * segment, in that case just let zero_bss allocate an empty buffer
> - * for it.
> + * Some segments may be completely empty, with a non-zero p_memsz
> + * but no backing file segment.
> */
> if (eppnt->p_filesz != 0) {
> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
> error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> MAP_PRIVATE | MAP_FIXED,
> image_fd, eppnt->p_offset - vaddr_po);
> @@ -2791,14 +2793,22 @@ static void load_elf_image(const char *image_name, int image_fd,
> if (error == -1) {
> goto exit_mmap;
> }
> - }
>
> - vaddr_ef = vaddr + eppnt->p_filesz;
> - vaddr_em = vaddr + eppnt->p_memsz;
> + /*
> + * If the load segment requests extra zeros (e.g. bss), map it.
> + */
> + if (eppnt->p_filesz < eppnt->p_memsz) {
> + zero_bss(vaddr_ef, vaddr_em, elf_prot);
> + }
> + } else if (eppnt->p_memsz != 0) {
> + vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_memsz + vaddr_po);
> + error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> + MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
> + -1, 0);
>
> - /* If the load segment requests extra zeros (e.g. bss), map it. */
> - if (vaddr_ef < vaddr_em) {
> - zero_bss(vaddr_ef, vaddr_em, elf_prot);
> + if (error == -1) {
> + goto exit_mmap;
> + }
> }
>
> /* Find the full program boundaries. */
>
Applied to my linux-user-for-6.0 branch.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-02-13 16:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 10:17 [PATCH] linux-user: Fix loading of BSS segments Giuseppe Musacchio
2020-12-17 10:48 ` Laurent Vivier
2020-12-17 10:59 ` Giuseppe Musacchio
2020-12-17 17:40 ` Stephen Long
2020-12-17 20:07 ` Laurent Vivier
2020-12-17 20:27 ` Stephen Long
2020-12-19 10:55 ` Laurent Vivier
2020-12-19 17:46 ` Giuseppe Musacchio
2021-02-13 16:41 ` Laurent Vivier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).