[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 05/11] linux-user: fix mmap/munmap/mprotect/m
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] [PATCH v3 05/11] linux-user: fix mmap/munmap/mprotect/mremap/shmat |
Date: |
Tue, 6 Mar 2018 18:39:57 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
Le 06/03/2018 à 18:28, Max Filippov a écrit :
> On Tue, Mar 6, 2018 at 4:02 AM, Laurent Vivier <address@hidden> wrote:
>> Le 01/03/2018 à 18:36, Max Filippov a écrit :
>>> In linux-user QEMU that runs for a target with TARGET_ABI_BITS bigger
>>> than L1_MAP_ADDR_SPACE_BITS an assertion in page_set_flags fires when
>>> mmap, munmap, mprotect, mremap or shmat is called for an address outside
>>> the guest address space. mmap and mprotect should return ENOMEM in such
>>> case.
>>>
>>> Introduce macro guest_range_valid that verifies if address range is
>>> within guest address space and does not wrap around. Use that macro in
>>> mmap/munmap/mprotect/mremap/shmat for error checking.
>>>
>>> Cc: address@hidden
>>> Cc: Riku Voipio <address@hidden>
>>> Cc: Laurent Vivier <address@hidden>
>>> Signed-off-by: Max Filippov <address@hidden>
>>> ---
>>> Changes v2->v3:
>>> - fix comparison in guest_valid: it must be 'less' to preserve the existing
>>> functionality, not 'less or equal'.
>>> - fix guest_range_valid: it may not use guest_valid, because single range
>>> that occupies all of the guest address space is valid.
>>>
>>> These changes fix assertion in page_check_range called from open_self_maps.
>>>
>>> include/exec/cpu-all.h | 2 +-
>>> include/exec/cpu_ldst.h | 12 +++++++-----
>>> linux-user/mmap.c | 20 +++++++++++++++-----
>>> linux-user/syscall.c | 3 +++
>>> 4 files changed, 26 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>>> index 0b141683f095..12bd049997ac 100644
>>> --- a/include/exec/cpu-all.h
>>> +++ b/include/exec/cpu-all.h
>>> @@ -160,7 +160,7 @@ extern int have_guest_base;
>>> extern unsigned long reserved_va;
>>>
>>> #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
>>> - (1ul << TARGET_VIRT_ADDR_SPACE_BITS) -
>>> 1)
>>> + (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)
>>
>> I don't understand why you do this change. Could you explain?
>
> For a lot of targets TARGET_VIRT_ADDR_SPACE_BITS is the same as
> HOST_LONG_BITS, so the shift results in undefined behavior. E.g. without
> this change I see the following warnings when building such targets for
> x86_64 host:
>
> linux-user/syscall.c:4905:10: note: in expansion of macro ‘guest_range_valid’
> if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
> ^~~~~~~~~~~~~~~~~
> include/exec/cpu-all.h:163:30: warning: left shift count >= width of
> type [-Wshift-count-overflow]
> (1ul << (TARGET_VIRT_ADDR_SPACE_BITS)) - 1)
>
> Changing it to 2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)
> fixes undefined behavior. Probably that deserves a comment in the
> changelog.
>
Perhaps you can keep the scheme used with Le 06/03/2018 à 18:28, Max
Filippov a écrit :
> On Tue, Mar 6, 2018 at 4:02 AM, Laurent Vivier <address@hidden> wrote:
>> Le 01/03/2018 à 18:36, Max Filippov a écrit :
>>> In linux-user QEMU that runs for a target with TARGET_ABI_BITS bigger
>>> than L1_MAP_ADDR_SPACE_BITS an assertion in page_set_flags fires when
>>> mmap, munmap, mprotect, mremap or shmat is called for an address outside
>>> the guest address space. mmap and mprotect should return ENOMEM in such
>>> case.
>>>
>>> Introduce macro guest_range_valid that verifies if address range is
>>> within guest address space and does not wrap around. Use that macro in
>>> mmap/munmap/mprotect/mremap/shmat for error checking.
>>>
>>> Cc: address@hidden
>>> Cc: Riku Voipio <address@hidden>
>>> Cc: Laurent Vivier <address@hidden>
>>> Signed-off-by: Max Filippov <address@hidden>
>>> ---
>>> Changes v2->v3:
>>> - fix comparison in guest_valid: it must be 'less' to preserve the
existing
>>> functionality, not 'less or equal'.
>>> - fix guest_range_valid: it may not use guest_valid, because single
range
>>> that occupies all of the guest address space is valid.
>>>
>>> These changes fix assertion in page_check_range called from
open_self_maps.
>>>
>>> include/exec/cpu-all.h | 2 +-
>>> include/exec/cpu_ldst.h | 12 +++++++-----
>>> linux-user/mmap.c | 20 +++++++++++++++-----
>>> linux-user/syscall.c | 3 +++
>>> 4 files changed, 26 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>>> index 0b141683f095..12bd049997ac 100644
>>> --- a/include/exec/cpu-all.h
>>> +++ b/include/exec/cpu-all.h
>>> @@ -160,7 +160,7 @@ extern int have_guest_base;
>>> extern unsigned long reserved_va;
>>>
>>> #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
>>> - (1ul <<
TARGET_VIRT_ADDR_SPACE_BITS) - 1)
>>> + (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)
>>
>> I don't understand why you do this change. Could you explain?
>
> For a lot of targets TARGET_VIRT_ADDR_SPACE_BITS is the same as
> HOST_LONG_BITS, so the shift results in undefined behavior. E.g. without
> this change I see the following warnings when building such targets for
> x86_64 host:
>
> linux-user/syscall.c:4905:10: note: in expansion of macro
‘guest_range_valid’
> if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
> ^~~~~~~~~~~~~~~~~
> include/exec/cpu-all.h:163:30: warning: left shift count >= width of
> type [-Wshift-count-overflow]
> (1ul << (TARGET_VIRT_ADDR_SPACE_BITS)) - 1)
>
> Changing it to 2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)
> fixes undefined behavior. Probably that deserves a comment in the
> changelog.
>
Perhaps you can keep the scheme used originally with h2g_valid(), it's
easier to read:
#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
#define GUEST_ADDR_MAX (reserved_va ? reserved_va : ~0ul)
#else
#define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
(1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
#endif
Thanks,
Laurent