I've slightly re-organised the check to more closely match the
sequence that the kernel uses in do_mmap().

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: umarcor <1783362@bugs.launchpad.net>
---
 linux-user/mmap.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index d0c50e4888..3ef69fa2d0 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -391,14 +391,22 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     }
 #endif
 
-    if (offset & ~TARGET_PAGE_MASK) {
+    if (!len) {
         errno = EINVAL;
         goto fail;
     }
 
     len = TARGET_PAGE_ALIGN(len);
-    if (len == 0)
-        goto the_end;
+    if (!len) {
+        errno = EINVAL;
+        goto fail;
+    }
+
+    if (offset & ~TARGET_PAGE_MASK) {
+        errno = EINVAL;
+        goto fail;
+    }
+
     real_start = start & qemu_host_page_mask;
     host_offset = offset & qemu_host_page_mask;
 
-- 
2.17.1

Le 26/07/2018 à 15:29, Alex Bennée a écrit :
> I've slightly re-organised the check to more closely match the
> sequence that the kernel uses in do_mmap().
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: umarcor <1783362@bugs.launchpad.net>
> ---
>  linux-user/mmap.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index d0c50e4888..3ef69fa2d0 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -391,14 +391,22 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>      }
>  #endif
>  
> -    if (offset & ~TARGET_PAGE_MASK) {
> +    if (!len) {
>          errno = EINVAL;
>          goto fail;
>      }
>  
>      len = TARGET_PAGE_ALIGN(len);
> -    if (len == 0)
> -        goto the_end;
> +    if (!len) {
> +        errno = EINVAL;
> +        goto fail;
> +    }

Why do you check twice len?
TARGET_PAGE_ALIGN() rounds up the value, so if it was not 0, it cannot
be now.

Thanks,
Laurent

Laurent Vivier <laurent@vivier.eu> writes:

> Le 26/07/2018 à 15:29, Alex Bennée a écrit:
>> I've slightly re-organised the check to more closely match the
>> sequence that the kernel uses in do_mmap().
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: umarcor <1783362@bugs.launchpad.net>
>> ---
>>  linux-user/mmap.c | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index d0c50e4888..3ef69fa2d0 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -391,14 +391,22 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>      }
>>  #endif
>>
>> -    if (offset & ~TARGET_PAGE_MASK) {
>> +    if (!len) {
>>          errno = EINVAL;
>>          goto fail;
>>      }
>>
>>      len = TARGET_PAGE_ALIGN(len);
>> -    if (len == 0)
>> -        goto the_end;
>> +    if (!len) {
>> +        errno = EINVAL;
>> +        goto fail;
>> +    }
>
> Why do you check twice len?
> TARGET_PAGE_ALIGN() rounds up the value, so if it was not 0, it cannot
> be now.

I was trying to follow the kernel style but I realise TARGET_PAGE_ALIGN
might be a different test compared to the kernel's PAGE_ALIGN(len) for
overflows:

	if (!len)
		return -EINVAL;

	/*
	 * Does the application expect PROT_READ to imply PROT_EXEC?
	 *
	 * (the exception is when the underlying filesystem is noexec
	 *  mounted, in which case we dont add PROT_EXEC.)
	 */
	if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
		if (!(file && path_noexec(&file->f_path)))
			prot |= PROT_EXEC;

	/* force arch specific MAP_FIXED handling in get_unmapped_area */
	if (flags & MAP_FIXED_NOREPLACE)
		flags |= MAP_FIXED;

	if (!(flags & MAP_FIXED))
		addr = round_hint_to_min(addr);

	/* Careful about overflows.. */
	len = PAGE_ALIGN(len);
	if (!len)
		return -ENOMEM;


>
> Thanks,
> Laurent


--
Alex Bennée