[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [PATCH v4 05/11] linux-user: fix mmap/munmap/mprotect/
From: |
Max Filippov |
Subject: |
Re: [Qemu-stable] [PATCH v4 05/11] linux-user: fix mmap/munmap/mprotect/mremap/shmat |
Date: |
Tue, 6 Mar 2018 15:20:30 -0800 |
On Tue, Mar 6, 2018 at 1:40 PM, Laurent Vivier <address@hidden> wrote:
> Le 06/03/2018 à 20:34, 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 v3->v4:
>> - change GUEST_ADDR_MAX and h2g_valid definitions as suggested by Laurent
>> Vivier.
>>
>> 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.
>>
>> include/exec/cpu-all.h | 4 ++++
>> include/exec/cpu_ldst.h | 14 ++++++++------
>> linux-user/mmap.c | 20 +++++++++++++++-----
>> linux-user/syscall.c | 3 +++
>> 4 files changed, 30 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
>> index 0b141683f095..6304cfa7e171 100644
>> --- a/include/exec/cpu-all.h
>> +++ b/include/exec/cpu-all.h
>> @@ -159,8 +159,12 @@ extern unsigned long guest_base;
>> extern int have_guest_base;
>> extern unsigned long reserved_va;
>>
>> +#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
>> +#define GUEST_ADDR_MAX (reserved_va ? reserved_va : ~0ul)
>
> In fact, below, for h2g_valid(), reserved_va is ignored in this case, so
> it should be:
>
> #define GUEST_ADDR_MAX (~0ul)
>
> [I know, my bad]
>
>> +#else
>> #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
>
> I think it should become "reserved_va ? reserved_va - 1 : \"
> as "reserved_va" is a size but GUEST_ADDR_MAX is the maximum value
> available. See below.
Agree.
>> (1ul << TARGET_VIRT_ADDR_SPACE_BITS) -
>> 1)
>> +#endif
>> #else
>>
>> #include "exec/hwaddr.h"
>> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
>> index 191f2e962a3c..22f5df9c8a92 100644
>> --- a/include/exec/cpu_ldst.h
>> +++ b/include/exec/cpu_ldst.h
>> @@ -52,15 +52,17 @@
>> #define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + guest_base))
>>
>> #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 guest_valid(x) ((x) < GUEST_ADDR_MAX)
>
> I think it should be ((x) <= GUEST_ADDR_MAX), because
>
> (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS))
> -> (__guest <= ((1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
> -> (__guest <= GUEST_ADDR_MAX)
Ok
> To work with reserved_va, it has also to be defined as "reserved_va -
> 1". And in open_self_maps() we should have "max = ... :
> (uintptr_t)g2h(GUEST_ADDR_MAX) + 1;" and then we have a "h2g(max - 1)"
> that will correctly return GUEST_ADDR_MAX.
Ok
> Then you don't need the "#if" because if "HOST_LONG_BITS <=
> TARGET_VIRT_ADDR_SPACE_BITS", x is 32bit and GUEST_ADDR_MAX) is ~0uk,
> and then ((x) <= GUEST_ADDR_MAX) is always true.
Ok.
>> #endif
>>
>> +#define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
>> +
>> +#define guest_range_valid(start, len) \
>> + ({unsigned long l = (len); \
>> + l <= GUEST_ADDR_MAX && (start) <= GUEST_ADDR_MAX - l; })
>> +
>> #define h2g_nocheck(x) ({ \
>> unsigned long __ret = (unsigned long)(x) - guest_base; \
>> (abi_ulong)__ret; \
> ...
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index e24f43c4a259..79245e73784f 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -4900,6 +4900,9 @@ static inline abi_ulong do_shmat(CPUArchState *cpu_env,
>> return -TARGET_EINVAL;
>> }
>> }
>> + if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
>
> if shmaddr is NULL, "the system chooses a suitable (unused) address" so
> you can't check this as is.
Why not? guest_range_valid will be true for shmaddr == NULL and all
valid sizes.
--
Thanks.
-- Max