Re: [PATCH v6 8/8] linux-user: Load pie executables at upper memory

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2023/08/02 17:42, Helge Deller wrote:
> On 8/2/23 09:49, Akihiko Odaki wrote:
>> On 2023/08/02 8:27, Helge Deller wrote:
>>> Fix the elf loader to calculate a valid TASK_UNMAPPED_BASE address 
>>> for all
>>> 32-bit architectures, based on the GUEST_ADDR_MAX constant.
>>>
>>> Additionally modify the elf loader to load dynamic pie executables at
>>> around:
>>> ~ 0x5500000000  for 64-bit guest binaries on 64-bit host,
>>> - 0x00300000    for 32-bit guest binaries on 64-bit host, and
>>> - 0x00000000    for 32-bit guest binaries on 32-bit host.
>>
>> Why do you change guest addresses depending on the host?
> 
> The addresses are guest-addresses.
> A 32-bit guest PIE can't be loaded at e.g. 0x5500000000,
> while a 64-bit guest PIE needs to be loaded at 0x5500000000.

I mean, why do you use address 0x00000000 for 32-bit guest binaries on 
32-bit host while you use address 0x00300000 on 64-bit host?

> 
>>> With this patch the Thread Sanitizer (TSan) application will work again,
>>> as in commit aab613fb9597 ("linux-user: Update TASK_UNMAPPED_BASE for
>>> aarch64").
>>>
>>> Signed-off-by: Helge Deller <deller@gmx.de>
>>> ---
>>>   linux-user/elfload.c |  6 ++++--
>>>   linux-user/loader.h  | 12 ++++++++++++
>>>   linux-user/mmap.c    | 35 ++++++++++++++++++-----------------
>>>   3 files changed, 34 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>>> index 47a118e430..8f5a79b537 100644
>>> --- a/linux-user/elfload.c
>>> +++ b/linux-user/elfload.c
>>> @@ -3021,6 +3021,7 @@ static void load_elf_image(const char 
>>> *image_name, int image_fd,
>>>       struct elfhdr *ehdr = (struct elfhdr *)bprm_buf;
>>>       struct elf_phdr *phdr;
>>>       abi_ulong load_addr, load_bias, loaddr, hiaddr, error;
>>> +    unsigned long load_offset = 0;
>>>       int i, retval, prot_exec;
>>>       Error *err = NULL;
>>>       bool is_main_executable;
>>> @@ -3121,6 +3122,7 @@ static void load_elf_image(const char 
>>> *image_name, int image_fd,
>>>                * select guest_base.  In this case we pass a size.
>>>                */
>>>               probe_guest_base(image_name, 0, hiaddr - loaddr);
>>> +            load_offset = TASK_UNMAPPED_BASE_PIE;
>>>           }
>>>       }
>>>
>>> @@ -3138,7 +3140,7 @@ static void load_elf_image(const char 
>>> *image_name, int image_fd,
>>>        * In both cases, we will overwrite pages in this range with 
>>> mappings
>>>        * from the executable.
>>>        */
>>> -    load_addr = target_mmap(loaddr, (size_t)hiaddr - loaddr + 1, 
>>> PROT_NONE,
>>> +    load_addr = target_mmap(loaddr + load_offset, (size_t)hiaddr - 
>>> loaddr + 1, PROT_NONE,
>>>                               MAP_PRIVATE | MAP_ANON | MAP_NORESERVE |
>>>                               (is_main_executable ? MAP_FIXED : 0),
>>>                               -1, 0);
>>> @@ -3176,7 +3178,7 @@ static void load_elf_image(const char 
>>> *image_name, int image_fd,
>>>       info->start_data = -1;
>>>       info->end_data = 0;
>>>       /* possible start for brk is behind all sections of this ELF 
>>> file. */
>>> -    info->brk = TARGET_PAGE_ALIGN(hiaddr);
>>> +    info->brk = TARGET_PAGE_ALIGN(load_offset + hiaddr);
>>>       info->elf_flags = ehdr->e_flags;
>>>
>>>       prot_exec = PROT_EXEC;
>>> diff --git a/linux-user/loader.h b/linux-user/loader.h
>>> index 59cbeacf24..3bbfc108eb 100644
>>> --- a/linux-user/loader.h
>>> +++ b/linux-user/loader.h
>>> @@ -18,6 +18,18 @@
>>>   #ifndef LINUX_USER_LOADER_H
>>>   #define LINUX_USER_LOADER_H
>>>
>>> +/* where to map binaries? */
>>> +#if HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 64
>>> +# define TASK_UNMAPPED_BASE_PIE 0x5500000000
>>> +# define TASK_UNMAPPED_BASE    0x7000000000
>>> +#elif HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 32
>>> +# define TASK_UNMAPPED_BASE_PIE    0x00300000
>>> +# define TASK_UNMAPPED_BASE    (GUEST_ADDR_MAX - 0x20000000 + 1)
>>> +#else /* HOST_LONG_BITS == 32 && TARGET_ABI_BITS == 32 */
>>> +# define TASK_UNMAPPED_BASE_PIE    0x00000000
>>> +# define TASK_UNMAPPED_BASE    0x40000000
>>> +#endif
>>> +
>>>   /*
>>>    * Read a good amount of data initially, to hopefully get all the
>>>    * program headers loaded.
>>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>>> index c624feead0..3441198e21 100644
>>> --- a/linux-user/mmap.c
>>> +++ b/linux-user/mmap.c
>>> @@ -23,6 +23,7 @@
>>>   #include "user-internals.h"
>>>   #include "user-mmap.h"
>>>   #include "target_mman.h"
>>> +#include "loader.h"
>>>
>>>   static pthread_mutex_t mmap_mutex = PTHREAD_MUTEX_INITIALIZER;
>>>   static __thread int mmap_lock_count;
>>> @@ -295,23 +296,6 @@ static bool mmap_frag(abi_ulong real_start, 
>>> abi_ulong start, abi_ulong last,
>>>       return true;
>>>   }
>>>
>>> -#if HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 64
>>> -#ifdef TARGET_AARCH64
>>> -# define TASK_UNMAPPED_BASE  0x5500000000
>>> -#else
>>> -# define TASK_UNMAPPED_BASE  0x4000000000
>>> -#endif
>>> -#elif HOST_LONG_BITS == 64 && TARGET_ABI_BITS == 32
>>> -#ifdef TARGET_HPPA
>>> -# define TASK_UNMAPPED_BASE  0xfa000000
>>> -#else
>>> -# define TASK_UNMAPPED_BASE  0xe0000000
>>> -#endif
>>> -#else /* HOST_LONG_BITS == 32 && TARGET_ABI_BITS == 32 */
>>> -# define TASK_UNMAPPED_BASE  0x40000000
>>> -#endif
>>> -abi_ulong mmap_next_start = TASK_UNMAPPED_BASE;
>>> -
>>>   unsigned long last_brk;
>>>
>>>   /*
>>> @@ -344,6 +328,23 @@ abi_ulong mmap_find_vma(abi_ulong start, 
>>> abi_ulong size, abi_ulong align)
>>>       abi_ulong addr;
>>>       int wrapped, repeat;
>>>
>>> +    static abi_ulong mmap_next_start;
>>> +
>>> +    /* initialize mmap_next_start if necessary */
>>> +    if (!mmap_next_start) {
>>> +        mmap_next_start = TASK_UNMAPPED_BASE;
>>> +
>>> +        /* do sanity checks on guest memory layout */
>>> +        if (mmap_next_start >= GUEST_ADDR_MAX) {
>>> +            mmap_next_start = GUEST_ADDR_MAX - 0x1000000000 + 1;
>>
>> What if GUEST_ADDR_MAX < 0x1000000000?
> 
> this check affects 64-bit executables only where GUEST_ADDR_MAX is bigger
> than that number. But I agree it's not directly visible from the code.
> 32-bit ones are taken care of where TASK_UNMAPPED_BASE is defined.
> 
>> I think you can just return a hard error when mmap_next_start >= 
>> GUEST_ADDR_MAX.
> 
> Can't happen, but I will rewrite it.
> 
>>> +        }
>>> +
>>> +        if (TASK_UNMAPPED_BASE_PIE >= mmap_next_start) {
>>> +            fprintf(stderr, "Memory too small for PIE executables.\n");
>>
>> Perhaps it's better to use error_report() for new code.
> 
> Ok.
> 
> Helge