[Qemu-devel] [PATCH v1 for 3.0 0/2] fix for bug 1783362

[Qemu-devel] [PATCH v1 for 3.0 0/2] fix for bug 1783362

[Alex Bennée]

Hi,

This fixes a bug where a zero len is passed to mmap. It comes with an
enhancement to the mmap test case.

Alex Bennée (2):
  linux-user/mmap.c: handle len = 0 maps correctly
  tests: add check_invalid_maps to test-mmap

 linux-user/mmap.c               | 14 +++++++++++---
 tests/tcg/multiarch/test-mmap.c | 16 +++++++++++++++-
 2 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly

[Alex Bennée]

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

Re: [Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly

[Laurent Vivier]

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

Re: [Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly

[Alex Bennée]

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

Re: [Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly

[Laurent Vivier]

Le 26/07/2018 à 19:58, Alex Bennée a écrit :
> 
> 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:
...
> 	/* Careful about overflows.. */
> 	len = PAGE_ALIGN(len);
> 	if (!len)
> 		return -ENOMEM;
> 


OK, so keep it but you should use ENOMEM, not EINVAL (and add a comment :) )

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly

[Alex Bennée]

Will do, thanks!

On Thu, 26 Jul 2018 at 19:12, Laurent Vivier <laurent@vivier.eu> wrote:

> Le 26/07/2018 à 19:58, Alex Bennée a écrit :
> >
> > 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:
> ...
> >       /* Careful about overflows.. */
> >       len = PAGE_ALIGN(len);
> >       if (!len)
> >               return -ENOMEM;
> >
>
>
> OK, so keep it but you should use ENOMEM, not EINVAL (and add a comment :)
> )
>
> Thanks,
> Laurent
>


-- 
Alex Bennée
KVM/QEMU Hacker for Linaro

[Qemu-devel] [PATCH v1 for 3.0 2/2] tests: add check_invalid_maps to test-mmap

[Alex Bennée]

This adds a test to make sure we fail properly for a 0 length mmap.
There are most likely other failure conditions we should also check.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: umarcor <1783362@bugs.launchpad.net>
---
 tests/tcg/multiarch/test-mmap.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 5c0afe6e49..7f62eba4e9 100644
--- a/tests/tcg/multiarch/test-mmap.c
+++ b/tests/tcg/multiarch/test-mmap.c
@@ -27,7 +27,7 @@
 #include <stdint.h>
 #include <string.h>
 #include <unistd.h>
-
+#include <errno.h>
 #include <sys/mman.h>
 
 #define D(x)
@@ -435,6 +435,19 @@ void checked_write(int fd, const void *buf, size_t count)
     fail_unless(rc == count);
 }
 
+void check_invalid_mmaps(void)
+{
+    unsigned char *addr;
+
+    /* Attempt to map a zero length page.  */
+    addr = mmap(NULL, 0, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    fprintf(stdout, "%s addr=%p", __func__, (void *)addr);
+    fail_unless(addr == MAP_FAILED);
+    fail_unless(errno == EINVAL);
+
+    fprintf(stdout, " passed\n");
+}
+
 int main(int argc, char **argv)
 {
 	char tempname[] = "/tmp/.cmmapXXXXXX";
@@ -476,6 +489,7 @@ int main(int argc, char **argv)
 	check_file_fixed_mmaps();
 	check_file_fixed_eof_mmaps();
 	check_file_unfixed_eof_mmaps();
+	check_invalid_mmaps();
 
 	/* Fails at the moment.  */
 	/* check_aligned_anonymous_fixed_mmaps_collide_with_host(); */
-- 
2.17.1

Re: [Qemu-devel] [PATCH v1 for 3.0 2/2] tests: add check_invalid_maps to test-mmap

[Richard Henderson]

On 07/26/2018 06:29 AM, Alex Bennée wrote:
> This adds a test to make sure we fail properly for a 0 length mmap.
> There are most likely other failure conditions we should also check.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: umarcor <1783362@bugs.launchpad.net>
> ---
>  tests/tcg/multiarch/test-mmap.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Might want to test length of e.g. -4 as well, which should get ENOMEM.


r~

Re: [Qemu-devel] [PATCH v1 for 3.0 0/2] fix for bug 1783362

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180726132947.28538-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [PATCH v1 for 3.0 0/2] fix for bug 1783362

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e00d491aae tests: add check_invalid_maps to test-mmap
08f3c02268 linux-user/mmap.c: handle len = 0 maps correctly

=== OUTPUT BEGIN ===
Checking PATCH 1/2: linux-user/mmap.c: handle len = 0 maps correctly...
Checking PATCH 2/2: tests: add check_invalid_maps to test-mmap...
ERROR: code indent should never use tabs
#53: FILE: tests/tcg/multiarch/test-mmap.c:492:
+^Icheck_invalid_mmaps();$

total: 1 errors, 0 warnings, 34 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v1 for 3.0 0/2] fix for bug 1783362

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180726132947.28538-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [PATCH v1 for 3.0 0/2] fix for bug 1783362

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
024d2b3da3 tests: add check_invalid_maps to test-mmap
a133b4a8b5 linux-user/mmap.c: handle len = 0 maps correctly

=== OUTPUT BEGIN ===
Checking PATCH 1/2: linux-user/mmap.c: handle len = 0 maps correctly...
Checking PATCH 2/2: tests: add check_invalid_maps to test-mmap...
ERROR: code indent should never use tabs
#54: FILE: tests/tcg/multiarch/test-mmap.c:492:
+^Icheck_invalid_mmaps();$

total: 1 errors, 0 warnings, 34 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com