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

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

[Alex Bennée]

Hi,

Updated to cover the overflow case properly as well.

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

 linux-user/mmap.c               | 15 ++++++++++++---
 tests/tcg/multiarch/test-mmap.c | 22 +++++++++++++++++++++-
 2 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 for 3.0 1/2] linux-user/mmap.c: handle invalid len 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(). We check for both the zero
case (EINVAL) and the overflow length case (ENOMEM).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: umarcor <1783362@bugs.launchpad.net>

---
v2
  - add comment on overflow
---
 linux-user/mmap.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index d0c50e4888..41e0983ce8 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -391,14 +391,23 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
     }
 #endif
 
-    if (offset & ~TARGET_PAGE_MASK) {
+    if (!len) {
         errno = EINVAL;
         goto fail;
     }
 
+    /* Also check for overflows... */
     len = TARGET_PAGE_ALIGN(len);
-    if (len == 0)
-        goto the_end;
+    if (!len) {
+        errno = ENOMEM;
+        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

[Qemu-devel] [PATCH v2 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Cc: umarcor <1783362@bugs.launchpad.net>

---
v2
  - add test for overflow
---
 tests/tcg/multiarch/test-mmap.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tests/tcg/multiarch/test-mmap.c b/tests/tcg/multiarch/test-mmap.c
index 5c0afe6e49..11d0e777b1 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,25 @@ 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);
+
+    /* Attempt to map a over length page.  */
+    addr = mmap(NULL, -4, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    fprintf(stdout, "%s addr=%p", __func__, (void *)addr);
+    fail_unless(addr == MAP_FAILED);
+    fail_unless(errno == ENOMEM);
+
+    fprintf(stdout, " passed\n");
+}
+
 int main(int argc, char **argv)
 {
 	char tempname[] = "/tmp/.cmmapXXXXXX";
@@ -476,6 +495,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 v2 for 3.0 1/2] linux-user/mmap.c: handle invalid len maps correctly

[Laurent Vivier]

Le 30/07/2018 à 15:43, 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(). We check for both the zero
> case (EINVAL) and the overflow length case (ENOMEM).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: umarcor <1783362@bugs.launchpad.net>
> 
> ---
> v2
>   - add comment on overflow
> ---
>  linux-user/mmap.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index d0c50e4888..41e0983ce8 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -391,14 +391,23 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>      }
>  #endif
>  
> -    if (offset & ~TARGET_PAGE_MASK) {
> +    if (!len) {
>          errno = EINVAL;
>          goto fail;
>      }
>  
> +    /* Also check for overflows... */
>      len = TARGET_PAGE_ALIGN(len);
> -    if (len == 0)
> -        goto the_end;
> +    if (!len) {
> +        errno = ENOMEM;
> +        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;
>  
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

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

[Alex Bennée]

Laurent Vivier <laurent@vivier.eu> writes:

> Le 30/07/2018 à 15:43, 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(). We check for both the zero
>> case (EINVAL) and the overflow length case (ENOMEM).
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: umarcor <1783362@bugs.launchpad.net>
>>
>> ---
>> v2
>>   - add comment on overflow
>> ---
>>  linux-user/mmap.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>> index d0c50e4888..41e0983ce8 100644
>> --- a/linux-user/mmap.c
>> +++ b/linux-user/mmap.c
>> @@ -391,14 +391,23 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>      }
>>  #endif
>>
>> -    if (offset & ~TARGET_PAGE_MASK) {
>> +    if (!len) {
>>          errno = EINVAL;
>>          goto fail;
>>      }
>>
>> +    /* Also check for overflows... */
>>      len = TARGET_PAGE_ALIGN(len);
>> -    if (len == 0)
>> -        goto the_end;
>> +    if (!len) {
>> +        errno = ENOMEM;
>> +        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;
>>
>>
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Are you going to take this via your queue or do you want me to re-post
with the r-b?

--
Alex Bennée

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

[Laurent Vivier]

Le 30/07/2018 à 16:21, Alex Bennée a écrit :
> 
> Laurent Vivier <laurent@vivier.eu> writes:
> 
>> Le 30/07/2018 à 15:43, 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(). We check for both the zero
>>> case (EINVAL) and the overflow length case (ENOMEM).
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: umarcor <1783362@bugs.launchpad.net>
>>>
>>> ---
>>> v2
>>>   - add comment on overflow
>>> ---
>>>  linux-user/mmap.c | 15 ++++++++++++---
>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>>> index d0c50e4888..41e0983ce8 100644
>>> --- a/linux-user/mmap.c
>>> +++ b/linux-user/mmap.c
>>> @@ -391,14 +391,23 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
>>>      }
>>>  #endif
>>>
>>> -    if (offset & ~TARGET_PAGE_MASK) {
>>> +    if (!len) {
>>>          errno = EINVAL;
>>>          goto fail;
>>>      }
>>>
>>> +    /* Also check for overflows... */
>>>      len = TARGET_PAGE_ALIGN(len);
>>> -    if (len == 0)
>>> -        goto the_end;
>>> +    if (!len) {
>>> +        errno = ENOMEM;
>>> +        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;
>>>
>>>
>>
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
> Are you going to take this via your queue or do you want me to re-post
> with the r-b?

I can take this via my queue.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2 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: 20180730134321.19898-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [PATCH v2 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'
4615f64664 tests: add check_invalid_maps to test-mmap
485d98a9f6 linux-user/mmap.c: handle invalid len maps correctly

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

total: 1 errors, 0 warnings, 40 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 v2 for 3.0 1/2] linux-user/mmap.c: handle invalid len maps correctly

[Richard Henderson]

On 07/30/2018 09:43 AM, Alex Bennée wrote:
> I've slightly re-organised the check to more closely match the
> sequence that the kernel uses in do_mmap(). We check for both the zero
> case (EINVAL) and the overflow length case (ENOMEM).
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: umarcor <1783362@bugs.launchpad.net>
> 
> ---
> v2
>   - add comment on overflow
> ---
>  linux-user/mmap.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)

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


r~

Re: [Qemu-devel] [PATCH v2 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: 20180730134321.19898-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [PATCH v2 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'
b48e0e0795 tests: add check_invalid_maps to test-mmap
b531edb72a linux-user/mmap.c: handle invalid len maps correctly

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

total: 1 errors, 0 warnings, 40 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