Re: [PATCH for-8.1 v10 04/14] linux-user: Use MAP_FIXED_NOREPLACE for initial image mmap

["Alex Bennée" <alex.bennee@linaro.org>]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2023/08/08 22:48, Alex Bennée wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>> 
>>> On 2023/08/08 18:43, Alex Bennée wrote:
>>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>>
>>>>> Use this as extra protection for the guest mapping over
>>>>> any qemu host mappings.
>>>>>
>>>>> Tested-by: Helge Deller <deller@gmx.de>
>>>>> Reviewed-by: Helge Deller <deller@gmx.de>
>>>>> Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>>> ---
>>>>>    linux-user/elfload.c | 9 ++++++---
>>>>>    1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>>>>> index 36e4026f05..1b4bb2d5af 100644
>>>>> --- a/linux-user/elfload.c
>>>>> +++ b/linux-user/elfload.c
>>>>> @@ -3147,8 +3147,11 @@ static void load_elf_image(const char *image_name, int image_fd,
>>>>>        /*
>>>>>         * Reserve address space for all of this.
>>>>>         *
>>>>> -     * In the case of ET_EXEC, we supply MAP_FIXED so that we get
>>>>> -     * exactly the address range that is required.
>>>>> +     * In the case of ET_EXEC, we supply MAP_FIXED_NOREPLACE so that we get
>>>>> +     * exactly the address range that is required.  Without reserved_va,
>>>>> +     * the guest address space is not isolated.  We have attempted to avoid
>>>>> +     * conflict with the host program itself via probe_guest_base, but using
>>>>> +     * MAP_FIXED_NOREPLACE instead of MAP_FIXED provides an extra check.
>>>>>         *
>>>>>         * Otherwise this is ET_DYN, and we are searching for a location
>>>>>         * that can hold the memory space required.  If the image is
>>>>> @@ -3160,7 +3163,7 @@ static void load_elf_image(const char *image_name, int image_fd,
>>>>>         */
>>>>>        load_addr = target_mmap(loaddr, (size_t)hiaddr - loaddr + 1, PROT_NONE,
>>>>>                                MAP_PRIVATE | MAP_ANON | MAP_NORESERVE |
>>>>> -                            (ehdr->e_type == ET_EXEC ? MAP_FIXED : 0),
>>>>> +                            (ehdr->e_type == ET_EXEC ? MAP_FIXED_NOREPLACE : 0),
>>>>>                                -1, 0);
>>>> We should probably also check the result == load_addr for the places
>>>> where MAP_FIXED_NOREPLACE isn't supported as we have this in osdep.h:
>>>>     #ifndef MAP_FIXED_NOREPLACE
>>>>     #define MAP_FIXED_NOREPLACE 0
>>>>     #endif
>>>> See 2667e069e7 (linux-user: don't use MAP_FIXED in
>>>> pgd_find_hole_fallback)
>>>
>>> It assumes target_mmap() emulates MAP_FIXED_NOREPLACE when the host
>>> does not support it as commit e69e032d1a ("linux-user: Use
>>> MAP_FIXED_NOREPLACE for do_brk()") already does, but defining
>>> MAP_FIXED_NOREPLACE zero breaks such emulation. I wrote a fix:
>>> https://patchew.org/QEMU/20230808115242.73025-1-akihiko.odaki@daynix.com/
>> Hmm doesn't that push the problem to real mmap() calls to a host
>> system
>> that doesn't support MAP_FIXED_NOREPLACE?
>
> That can happen even without that patch if you run QEMU built with a
> new libc on old Linux so we should have prepared for that kind of
> situation.
>
> The man page also says:
>> Note that older kernels which do not recognize the MAP_FIXED_NOREPLACE
>> flag will typically (upon detecting a collision with a preexisting
>> mapping) fall back to a “non-MAP_FIXED” type of behavior: they will
>> return an address that is different from the requested address.
>> Therefore, backward-compatible software should check the returned
>> address against the requested address.
> https://man7.org/linux/man-pages/man2/mmap.2.html
>
> It basically means MAP_FIXED_NOREPLACE has no effect on a host that
> doesn't support it, and the existing code checking the returned
> address should continue to work.

OK - as long as it doesn't barf on unknown flags.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro