Re: [Qemu-devel] [PATCH v3 05/11] linux-user: fix mmap/munmap/mprotect/mremap/shmat

[Laurent Vivier]

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