On Tue, Mar 6, 2018 at 4:02 AM, Laurent Vivier <laur...@vivier.eu> 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: qemu-sta...@nongnu.org
>> Cc: Riku Voipio <riku.voi...@iki.fi>
>> Cc: Laurent Vivier <laur...@vivier.eu>
>> Signed-off-by: Max Filippov <jcmvb...@gmail.com>
>> ---
>> 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.

>>  #else
>>
>>  #include "exec/hwaddr.h"
>> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
>> index 191f2e962a3c..6528fd829ffe 100644
>> --- a/include/exec/cpu_ldst.h
>> +++ b/include/exec/cpu_ldst.h
>> @@ -53,14 +53,16 @@
>>
>>  #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
>>  #define h2g_valid(x) 1
>> +#define guest_valid(x) 1
>>  #else
>> -#define h2g_valid(x) ({ \
>> -    unsigned long __guest = (unsigned long)(x) - guest_base; \
>> -    (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS)) && \
>> -    (!reserved_va || (__guest < reserved_va)); \
>> -})
>> +#define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
>> +#define guest_valid(x) ((x) < GUEST_ADDR_MAX)
>>  #endif
>
> I think you can just define h2g_valid(x) after the "endif":
>
>   #define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
>
> it will be set according to the guest_valid() definition ("1" or the
> result of the evaluation).

Ok, will do.

-- 
Thanks.
-- Max

Reply via email to