diff mbox series

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

Message ID 20180726132947.28538-2-alex.bennee@linaro.org
State New
Headers show
Series fix for bug 1783362 | expand

Commit Message

Alex Bennée July 26, 2018, 1:29 p.m. UTC 
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(-)

-- 
2.17.1

Comments

Laurent Vivier July 26, 2018, 1:33 p.m. UTC | #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
Alex Bennée July 26, 2018, 5:58 p.m. UTC | #2 
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
Laurent Vivier July 26, 2018, 6:12 p.m. UTC | #3 
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 July 26, 2018, 6:52 p.m. UTC | #4 
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
diff mbox series

Patch

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;