Re: [Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly
Will do, thanks!
On Thu, 26 Jul 2018 at 19:12, Laurent Vivier wrote:
> Le 26/07/2018 à 19:58, Alex Bennée a écrit :
> >
> > Laurent Vivier 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
> >>> Cc: umarcor <1783...@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
Re: [Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly
Le 26/07/2018 à 19:58, Alex Bennée a écrit :
>
> Laurent Vivier 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
>>> Cc: umarcor <1783...@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
Laurent Vivier 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
>> Cc: umarcor <1783...@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(>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
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
> Cc: umarcor <1783...@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
