In load_elf_binary, struct image_info interp_info is used without being
properly initialized.  One result is that when the ELF's program header
doesn't contain an entry for the ABI flags, then the value of the struct
image_info's fp_abi field is set to whatever happened to be in stack
memory at the time.

This patch both sanitizes interp_info and initializes fp_abi for
TARGET_MIPS to MIPS_ABI_FP_UNKNOWN so that when we don't know the FP
ABI, we don't just blow up.  Currently, this bug is a complete stopper
for some MIPS binaries.

***PLEASE NOTE***
There may be other bugs as a result of struct image_info interp_info
fields not being properly initialized -- this patch only addresses the
fp_abi field.  I reccomend somebody who knows the code better than I
audit this function and the whole of that execution path.

Fixes bug #1825002 and affects 3.1.0 and 4.x, reccomend backporting to
3.1.0.

Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
---
 linux-user/elfload.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index c1a26021f8..7f09d572a2 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2698,6 +2698,11 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
     char *elf_interpreter = NULL;
     char *scratch;
 
+    memset(&interp_info, 0, sizeof(interp_info));
+#ifdef TARGET_MIPS
+    interp_info.fp_abi = MIPS_ABI_FP_UNKNOWN;
+#endif
+
     info->start_mmap = (abi_ulong)ELF_START_MMAP;
 
     load_elf_image(bprm->filename, bprm->fd, info,
-- 
2.19.2

On Wed, Apr 17, 2019 at 9:50 PM Daniel Santos <daniel.santos@pobox.com> wrote:
>
> In load_elf_binary, struct image_info interp_info is used without being
> properly initialized.  One result is that when the ELF's program header
> doesn't contain an entry for the ABI flags, then the value of the struct
> image_info's fp_abi field is set to whatever happened to be in stack
> memory at the time.
>
> This patch both sanitizes interp_info and initializes fp_abi for
> TARGET_MIPS to MIPS_ABI_FP_UNKNOWN so that when we don't know the FP
> ABI, we don't just blow up.  Currently, this bug is a complete stopper
> for some MIPS binaries.
>
> ***PLEASE NOTE***
> There may be other bugs as a result of struct image_info interp_info
> fields not being properly initialized -- this patch only addresses the
> fp_abi field.  I reccomend somebody who knows the code better than I
> audit this function and the whole of that execution path.
>
> Fixes bug #1825002 and affects 3.1.0 and 4.x, reccomend backporting to
> 3.1.0.
>
> Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
> ---

Daniel, not knowing that you already send this patch, I included it in another
series (with different title and commit message, but the same content):

https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03813.html

Please let's track this patch there.

I will change the commit message to bring it closer to the yours version
in the next version of the series, and I will review the patch from MIPS
point of view.

Just advice for the future: Before sending patches to qemu-devel,
check what are maintainers for the applicable code. There is even a
script for that: <qemu-root>/scripts/get_maintainers.pl

There are also other rules and conventiones, and all of them are
mentioned on the page "How to submit a patch" on QEMU web site.

But, in any case, many thanks for discovering and reporting the bug,
and even devising the fix!

Yours,
Aleksandar

>  linux-user/elfload.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index c1a26021f8..7f09d572a2 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2698,6 +2698,11 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
>      char *elf_interpreter = NULL;
>      char *scratch;
>
> +    memset(&interp_info, 0, sizeof(interp_info));
> +#ifdef TARGET_MIPS
> +    interp_info.fp_abi = MIPS_ABI_FP_UNKNOWN;
> +#endif
> +
>      info->start_mmap = (abi_ulong)ELF_START_MMAP;
>
>      load_elf_image(bprm->filename, bprm->fd, info,
> --
> 2.19.2
>
>

Hello,

Sorry for my slow reply.  I don't mind that, as long as I end up being
shown as the author in git. :)  I've never committed from an email
before, so I'm not sure how that works.  Does adding another "From: "
header in the body patch that up with git-am?

I don't know how much I'll be contributing to qemu in the future --
there are a few crazy things I have considered trying to add and I'll be
using qemu quite a bit in the future, so this probably won't be my only
patch.  But in this case, I just *really* needed to get something MIPS
running on my workstation.

Also, as my big "please note", it would be good for somebody more
familiar with that code to make sure anything else that isn't explicitly
initialized will behave correctly after being memset to zero.  When a
zero is OK, I often add an explicit this.that = 0; to clarify the
intention and just expect gcc to compile it away.

Thanks!
Daniel

On 4/23/19 1:00 PM, Aleksandar Markovic wrote:
> On Wed, Apr 17, 2019 at 9:50 PM Daniel Santos <daniel.santos@pobox.com> wrote:
>> In load_elf_binary, struct image_info interp_info is used without being
>> properly initialized.  One result is that when the ELF's program header
>> doesn't contain an entry for the ABI flags, then the value of the struct
>> image_info's fp_abi field is set to whatever happened to be in stack
>> memory at the time.
>>
>> This patch both sanitizes interp_info and initializes fp_abi for
>> TARGET_MIPS to MIPS_ABI_FP_UNKNOWN so that when we don't know the FP
>> ABI, we don't just blow up.  Currently, this bug is a complete stopper
>> for some MIPS binaries.
>>
>> ***PLEASE NOTE***
>> There may be other bugs as a result of struct image_info interp_info
>> fields not being properly initialized -- this patch only addresses the
>> fp_abi field.  I reccomend somebody who knows the code better than I
>> audit this function and the whole of that execution path.
>>
>> Fixes bug #1825002 and affects 3.1.0 and 4.x, reccomend backporting to
>> 3.1.0.
>>
>> Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
>> ---
> Daniel, not knowing that you already send this patch, I included it in another
> series (with different title and commit message, but the same content):
>
> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg03813.html
>
> Please let's track this patch there.
>
> I will change the commit message to bring it closer to the yours version
> in the next version of the series, and I will review the patch from MIPS
> point of view.
>
> Just advice for the future: Before sending patches to qemu-devel,
> check what are maintainers for the applicable code. There is even a
> script for that: <qemu-root>/scripts/get_maintainers.pl
>
> There are also other rules and conventiones, and all of them are
> mentioned on the page "How to submit a patch" on QEMU web site.
>
> But, in any case, many thanks for discovering and reporting the bug,
> and even devising the fix!
>
> Yours,
> Aleksandar
>
>>  linux-user/elfload.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index c1a26021f8..7f09d572a2 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -2698,6 +2698,11 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
>>      char *elf_interpreter = NULL;
>>      char *scratch;
>>
>> +    memset(&interp_info, 0, sizeof(interp_info));
>> +#ifdef TARGET_MIPS
>> +    interp_info.fp_abi = MIPS_ABI_FP_UNKNOWN;
>> +#endif
>> +
>>      info->start_mmap = (abi_ulong)ELF_START_MMAP;
>>
>>      load_elf_image(bprm->filename, bprm->fd, info,
>> --
>> 2.19.2
>>
>>