[PATCH v3 0/4] Add overflow checks for several syscalls

[PATCH v3 0/4] Add overflow checks for several syscalls

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
The return value of mlock is zero. But nothing will be locked since the
len in do_mlock overflows to zero due to the following code in mlock:

  len = PAGE_ALIGN(len + (offset_in_page(start)));

The same problem happens in munlock.

Add new check and return -EINVAL to fix this overflowing scenarios since
they are absolutely wrong.

Similar logic is used to fix problems with multiple syscalls.

Changelog since v2[2]:
- modified the way of checking overflows based on Andrew's comments

Changelog since v1[1]:
- only check overflow rather than access_ok to keep backward-compatibility

[1]: https://lore.kernel.org/lkml/20221228141701.c64add46c4b09aa17f605baf@linux-foundation.org/T/
[2]: https://lore.kernel.org/linux-mm/20230116115813.2956935-5-mawupeng1@huawei.com/T/

Ma Wupeng (4):
  mm/mlock: return EINVAL if len overflows for mlock/munlock
  mm/mempolicy: return EINVAL for if len overflows for
    set_mempolicy_home_node
  mm/mempolicy: return EINVAL if len overflows for mbind
  mm/msync: return ENOMEM if len overflows for msync

 mm/mempolicy.c | 18 ++++++++++++------
 mm/mlock.c     | 14 ++++++++++++--
 mm/msync.c     |  9 ++++++---
 3 files changed, 30 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v3 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
The return value of mlock is zero. But nothing will be locked since the
len in do_mlock overflows to zero due to the following code in mlock:

  len = PAGE_ALIGN(len + (offset_in_page(start)));

The same problem happens in munlock.

Add new check and return -EINVAL to fix this overflowing scenarios since
they are absolutely wrong.

Return 0 early to avoid burn a bunch of cpu cycles if len == 0.

Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
---
 mm/mlock.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/mm/mlock.c b/mm/mlock.c
index 7032f6dd0ce1..eb09968ba27f 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
 	end = start + len;
 	if (end < start)
 		return -EINVAL;
-	if (end == start)
-		return 0;
 	vma = mas_walk(&mas);
 	if (!vma)
 		return -ENOMEM;
@@ -575,7 +573,13 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
 	if (!can_do_mlock())
 		return -EPERM;
 
+	if (!len)
+		return 0;
+
 	len = PAGE_ALIGN(len + (offset_in_page(start)));
+	if (!len)
+		return -EINVAL;
+
 	start &= PAGE_MASK;
 
 	lock_limit = rlimit(RLIMIT_MEMLOCK);
@@ -635,7 +639,13 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
 
 	start = untagged_addr(start);
 
+	if (!len)
+		return 0;
+
 	len = PAGE_ALIGN(len + (offset_in_page(start)));
+	if (!len)
+		return -EINVAL;
+
 	start &= PAGE_MASK;
 
 	if (mmap_write_lock_killable(current->mm))
-- 
2.25.1

Re: [PATCH v3 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock

[David Hildenbrand]

On 28.01.23 07:32, Wupeng Ma wrote:
> From: Ma Wupeng <mawupeng1@huawei.com>
> 
> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
> The return value of mlock is zero. But nothing will be locked since the
> len in do_mlock overflows to zero due to the following code in mlock:
> 
>    len = PAGE_ALIGN(len + (offset_in_page(start)));
> 
> The same problem happens in munlock.
> 
> Add new check and return -EINVAL to fix this overflowing scenarios since
> they are absolutely wrong.
> 
> Return 0 early to avoid burn a bunch of cpu cycles if len == 0.
> 
> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>   mm/mlock.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 7032f6dd0ce1..eb09968ba27f 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>   	end = start + len;
>   	if (end < start)
>   		return -EINVAL;
> -	if (end == start)
> -		return 0;
>   	vma = mas_walk(&mas);
>   	if (!vma)
>   		return -ENOMEM;
> @@ -575,7 +573,13 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>   	if (!can_do_mlock())
>   		return -EPERM;
>   
> +	if (!len)
> +		return 0;
> +
>   	len = PAGE_ALIGN(len + (offset_in_page(start)));
> +	if (!len)
> +		return -EINVAL;
> +
>   	start &= PAGE_MASK;

The "ordinary" overflows are detected in apply_vma_lock_flags(), correct?

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock

[mawupeng]

On 2023/2/4 1:14, David Hildenbrand wrote:
> On 28.01.23 07:32, Wupeng Ma wrote:
>> From: Ma Wupeng <mawupeng1@huawei.com>
>>
>> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
>> The return value of mlock is zero. But nothing will be locked since the
>> len in do_mlock overflows to zero due to the following code in mlock:
>>
>>    len = PAGE_ALIGN(len + (offset_in_page(start)));
>>
>> The same problem happens in munlock.
>>
>> Add new check and return -EINVAL to fix this overflowing scenarios since
>> they are absolutely wrong.
>>
>> Return 0 early to avoid burn a bunch of cpu cycles if len == 0.
>>
>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   mm/mlock.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 7032f6dd0ce1..eb09968ba27f 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>>       end = start + len;
>>       if (end < start)
>>           return -EINVAL;
>> -    if (end == start)
>> -        return 0;
>>       vma = mas_walk(&mas);
>>       if (!vma)
>>           return -ENOMEM;
>> @@ -575,7 +573,13 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>       if (!can_do_mlock())
>>           return -EPERM;
>>   +    if (!len)
>> +        return 0;
>> +
>>       len = PAGE_ALIGN(len + (offset_in_page(start)));
>> +    if (!len)
>> +        return -EINVAL;
>> +
>>       start &= PAGE_MASK;
> 
> The "ordinary" overflows are detected in apply_vma_lock_flags(), correct?

Overflow is not checked anywhere however the ordinary return early if len == 0 is detected in apply_vma_lock_flags().

do_mlock
  apply_vma_lock_flags
	end = start + len;
	if (end == start)
	  return 0;

Move the checking to the begin is easier to detect overflows and make the logic clearer
and avoid burn a bunch of cpu cycles.

> 

Re: [PATCH v3 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock

[David Hildenbrand]

On 06.02.23 01:48, mawupeng wrote:
> 
> 
> On 2023/2/4 1:14, David Hildenbrand wrote:
>> On 28.01.23 07:32, Wupeng Ma wrote:
>>> From: Ma Wupeng <mawupeng1@huawei.com>
>>>
>>> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
>>> The return value of mlock is zero. But nothing will be locked since the
>>> len in do_mlock overflows to zero due to the following code in mlock:
>>>
>>>     len = PAGE_ALIGN(len + (offset_in_page(start)));
>>>
>>> The same problem happens in munlock.
>>>
>>> Add new check and return -EINVAL to fix this overflowing scenarios since
>>> they are absolutely wrong.
>>>
>>> Return 0 early to avoid burn a bunch of cpu cycles if len == 0.
>>>
>>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>>> ---
>>>    mm/mlock.c | 14 ++++++++++++--
>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>> index 7032f6dd0ce1..eb09968ba27f 100644
>>> --- a/mm/mlock.c
>>> +++ b/mm/mlock.c
>>> @@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>>>        end = start + len;
>>>        if (end < start)
>>>            return -EINVAL;
>>> -    if (end == start)
>>> -        return 0;
>>>        vma = mas_walk(&mas);
>>>        if (!vma)
>>>            return -ENOMEM;
>>> @@ -575,7 +573,13 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>>        if (!can_do_mlock())
>>>            return -EPERM;
>>>    +    if (!len)
>>> +        return 0;
>>> +
>>>        len = PAGE_ALIGN(len + (offset_in_page(start)));
>>> +    if (!len)
>>> +        return -EINVAL;
>>> +
>>>        start &= PAGE_MASK;
>>
>> The "ordinary" overflows are detected in apply_vma_lock_flags(), correct?
> 
> Overflow is not checked anywhere however the ordinary return early if len == 0 is detected in apply_vma_lock_flags().
> 

I meant the

end = start + len;
if (end < start)
	return -EINVAL;

Essentially, what I wanted to double-check is that with your changes, we 
catch all kinds of overflows as documented in the man page, correct?

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock

[mawupeng]

On 2023/2/7 1:05, David Hildenbrand wrote:
> On 06.02.23 01:48, mawupeng wrote:
>>
>>
>> On 2023/2/4 1:14, David Hildenbrand wrote:
>>> On 28.01.23 07:32, Wupeng Ma wrote:
>>>> From: Ma Wupeng <mawupeng1@huawei.com>
>>>>
>>>> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
>>>> The return value of mlock is zero. But nothing will be locked since the
>>>> len in do_mlock overflows to zero due to the following code in mlock:
>>>>
>>>>     len = PAGE_ALIGN(len + (offset_in_page(start)));
>>>>
>>>> The same problem happens in munlock.
>>>>
>>>> Add new check and return -EINVAL to fix this overflowing scenarios since
>>>> they are absolutely wrong.
>>>>
>>>> Return 0 early to avoid burn a bunch of cpu cycles if len == 0.
>>>>
>>>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>>>> ---
>>>>    mm/mlock.c | 14 ++++++++++++--
>>>>    1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>> index 7032f6dd0ce1..eb09968ba27f 100644
>>>> --- a/mm/mlock.c
>>>> +++ b/mm/mlock.c
>>>> @@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>>>>        end = start + len;
>>>>        if (end < start)
>>>>            return -EINVAL;
>>>> -    if (end == start)
>>>> -        return 0;
>>>>        vma = mas_walk(&mas);
>>>>        if (!vma)
>>>>            return -ENOMEM;
>>>> @@ -575,7 +573,13 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>>>        if (!can_do_mlock())
>>>>            return -EPERM;
>>>>    +    if (!len)
>>>> +        return 0;
>>>> +
>>>>        len = PAGE_ALIGN(len + (offset_in_page(start)));
>>>> +    if (!len)
>>>> +        return -EINVAL;
>>>> +
>>>>        start &= PAGE_MASK;
>>>
>>> The "ordinary" overflows are detected in apply_vma_lock_flags(), correct?
>>
>> Overflow is not checked anywhere however the ordinary return early if len == 0 is detected in apply_vma_lock_flags().
>>
> 
> I meant the
> 
> end = start + len;
> if (end < start)
>     return -EINVAL;
> 
> Essentially, what I wanted to double-check is that with your changes, we catch all kinds of overflows as documented in the man page, correct?

Oh i see. You are right, The "ordinary" overflows are detected for mlock/munlock in apply_vma_lock_flags().

Yes, we may need to update the man page for all these four syscalls.

Thanks, 

mawupeng.


> 

Re: [PATCH v3 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock

[David Hildenbrand]

On 07.02.23 02:24, mawupeng wrote:
> 
> 
> On 2023/2/7 1:05, David Hildenbrand wrote:
>> On 06.02.23 01:48, mawupeng wrote:
>>>
>>>
>>> On 2023/2/4 1:14, David Hildenbrand wrote:
>>>> On 28.01.23 07:32, Wupeng Ma wrote:
>>>>> From: Ma Wupeng <mawupeng1@huawei.com>
>>>>>
>>>>> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
>>>>> The return value of mlock is zero. But nothing will be locked since the
>>>>> len in do_mlock overflows to zero due to the following code in mlock:
>>>>>
>>>>>      len = PAGE_ALIGN(len + (offset_in_page(start)));
>>>>>
>>>>> The same problem happens in munlock.
>>>>>
>>>>> Add new check and return -EINVAL to fix this overflowing scenarios since
>>>>> they are absolutely wrong.
>>>>>
>>>>> Return 0 early to avoid burn a bunch of cpu cycles if len == 0.
>>>>>
>>>>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>>>>> ---
>>>>>     mm/mlock.c | 14 ++++++++++++--
>>>>>     1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>>> index 7032f6dd0ce1..eb09968ba27f 100644
>>>>> --- a/mm/mlock.c
>>>>> +++ b/mm/mlock.c
>>>>> @@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>>>>>         end = start + len;
>>>>>         if (end < start)
>>>>>             return -EINVAL;
>>>>> -    if (end == start)
>>>>> -        return 0;
>>>>>         vma = mas_walk(&mas);
>>>>>         if (!vma)
>>>>>             return -ENOMEM;
>>>>> @@ -575,7 +573,13 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>>>>         if (!can_do_mlock())
>>>>>             return -EPERM;
>>>>>     +    if (!len)
>>>>> +        return 0;
>>>>> +
>>>>>         len = PAGE_ALIGN(len + (offset_in_page(start)));
>>>>> +    if (!len)
>>>>> +        return -EINVAL;
>>>>> +
>>>>>         start &= PAGE_MASK;
>>>>
>>>> The "ordinary" overflows are detected in apply_vma_lock_flags(), correct?
>>>
>>> Overflow is not checked anywhere however the ordinary return early if len == 0 is detected in apply_vma_lock_flags().
>>>
>>
>> I meant the
>>
>> end = start + len;
>> if (end < start)
>>      return -EINVAL;
>>
>> Essentially, what I wanted to double-check is that with your changes, we catch all kinds of overflows as documented in the man page, correct?
> 
> Oh i see. You are right, The "ordinary" overflows are detected for mlock/munlock in apply_vma_lock_flags().
> 
> Yes, we may need to update the man page for all these four syscalls.

E.g., mlock() already documents "EINVAL (mlock(),  mlock2(),  and 
munlock()) The result of the addition addr+len was less than addr (e.g., 
the addition may have resulted in an overflow)."

Just to rephrase my question what I wanted to double-check: are we now 
identifying all such overflows or are you aware of other corner cases?

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock

[mawupeng]

On 2023/2/8 21:51, David Hildenbrand wrote:
> On 07.02.23 02:24, mawupeng wrote:
>>
>>
>> On 2023/2/7 1:05, David Hildenbrand wrote:
>>> On 06.02.23 01:48, mawupeng wrote:
>>>>
>>>>
>>>> On 2023/2/4 1:14, David Hildenbrand wrote:
>>>>> On 28.01.23 07:32, Wupeng Ma wrote:
>>>>>> From: Ma Wupeng <mawupeng1@huawei.com>
>>>>>>
>>>>>> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
>>>>>> The return value of mlock is zero. But nothing will be locked since the
>>>>>> len in do_mlock overflows to zero due to the following code in mlock:
>>>>>>
>>>>>>      len = PAGE_ALIGN(len + (offset_in_page(start)));
>>>>>>
>>>>>> The same problem happens in munlock.
>>>>>>
>>>>>> Add new check and return -EINVAL to fix this overflowing scenarios since
>>>>>> they are absolutely wrong.
>>>>>>
>>>>>> Return 0 early to avoid burn a bunch of cpu cycles if len == 0.
>>>>>>
>>>>>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>>>>>> ---
>>>>>>     mm/mlock.c | 14 ++++++++++++--
>>>>>>     1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>>>>> index 7032f6dd0ce1..eb09968ba27f 100644
>>>>>> --- a/mm/mlock.c
>>>>>> +++ b/mm/mlock.c
>>>>>> @@ -478,8 +478,6 @@ static int apply_vma_lock_flags(unsigned long start, size_t len,
>>>>>>         end = start + len;
>>>>>>         if (end < start)
>>>>>>             return -EINVAL;
>>>>>> -    if (end == start)
>>>>>> -        return 0;
>>>>>>         vma = mas_walk(&mas);
>>>>>>         if (!vma)
>>>>>>             return -ENOMEM;
>>>>>> @@ -575,7 +573,13 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>>>>>         if (!can_do_mlock())
>>>>>>             return -EPERM;
>>>>>>     +    if (!len)
>>>>>> +        return 0;
>>>>>> +
>>>>>>         len = PAGE_ALIGN(len + (offset_in_page(start)));
>>>>>> +    if (!len)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>>         start &= PAGE_MASK;
>>>>>
>>>>> The "ordinary" overflows are detected in apply_vma_lock_flags(), correct?
>>>>
>>>> Overflow is not checked anywhere however the ordinary return early if len == 0 is detected in apply_vma_lock_flags().
>>>>
>>>
>>> I meant the
>>>
>>> end = start + len;
>>> if (end < start)
>>>      return -EINVAL;
>>>
>>> Essentially, what I wanted to double-check is that with your changes, we catch all kinds of overflows as documented in the man page, correct?
>>
>> Oh i see. You are right, The "ordinary" overflows are detected for mlock/munlock in apply_vma_lock_flags().
>>
>> Yes, we may need to update the man page for all these four syscalls.
> 
> E.g., mlock() already documents "EINVAL (mlock(),  mlock2(),  and munlock()) The result of the addition addr+len was less than addr (e.g., the addition may have resulted in an overflow)."
> 
> Just to rephrase my question what I wanted to double-check: are we now identifying all such overflows or are you aware of other corner cases?

AFAICT. There is no cornel cases now.

Pervious normal overflow can be detected via the following codes as you mentioned.

end = start + len;
if (end < start)
	return -EINVAL;
this is fine for normal overflows.

But the len may be zero in PAGE_ALIGN(len + (offset_in_page(start))).

This leed to two scenarios for len == 0:
a) user pass len as zero. this is fine, we don't need to do anything.
b) overflow scenarios. we need to return err rather than response ok as above.

> 

[PATCH v3 2/4] mm/mempolicy: return EINVAL for if len overflows for set_mempolicy_home_node

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

Check and return 0 if len == 0 at the beginning of the function.
Return -EINVAL if len overflows for set_mempolicy_home_node.

Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
---
 mm/mempolicy.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 02c8a712282f..85c5d3c2503b 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1512,13 +1512,16 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
 	if (home_node >= MAX_NUMNODES || !node_online(home_node))
 		return -EINVAL;
 
+	if (!len)
+		return 0;
+
 	len = PAGE_ALIGN(len);
-	end = start + len;
+	if (!len)
+		return -EINVAL;
 
+	end = start + len;
 	if (end < start)
 		return -EINVAL;
-	if (end == start)
-		return 0;
 	mmap_write_lock(mm);
 	for_each_vma_range(vmi, vma, end) {
 		vmstart = max(start, vma->vm_start);
-- 
2.25.1

[PATCH v3 3/4] mm/mempolicy: return EINVAL if len overflows for mbind

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

Check and return 0 if len == 0 at the beginning of the function.
Return -EINVAL if len overflows for mbind.

Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
---
 mm/mempolicy.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 85c5d3c2503b..7791be5a2677 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1275,13 +1275,16 @@ static long do_mbind(unsigned long start, unsigned long len,
 	if (mode == MPOL_DEFAULT)
 		flags &= ~MPOL_MF_STRICT;
 
+	if (!len)
+		return 0;
+
 	len = PAGE_ALIGN(len);
-	end = start + len;
+	if (!len)
+		return -EINVAL;
 
+	end = start + len;
 	if (end < start)
 		return -EINVAL;
-	if (end == start)
-		return 0;
 
 	new = mpol_new(mode, mode_flags, nmask);
 	if (IS_ERR(new))
-- 
2.25.1

[PATCH v3 4/4] mm/msync: return ENOMEM if len overflows for msync

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

Check and return 0 if len == 0 at the beginning of the function.
Return -ENOMEM if len overflows for msync.

Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
---
 mm/msync.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/msync.c b/mm/msync.c
index ac4c9bfea2e7..3104c97d70d3 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -46,13 +46,16 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
 	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
 		goto out;
 	error = -ENOMEM;
+	if (!len)
+		return 0;
+
 	len = (len + ~PAGE_MASK) & PAGE_MASK;
+	if (!len)
+		goto out;
+
 	end = start + len;
 	if (end < start)
 		goto out;
-	error = 0;
-	if (end == start)
-		goto out;
 	/*
 	 * If the interval [start,end) covers some unmapped address ranges,
 	 * just ignore them, but return -ENOMEM at the end. Besides, if the
-- 
2.25.1

Re: [PATCH v3 0/4] Add overflow checks for several syscalls

[David Hildenbrand]

On 28.01.23 07:32, Wupeng Ma wrote:
> From: Ma Wupeng <mawupeng1@huawei.com>
> 
> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
> The return value of mlock is zero. But nothing will be locked since the
> len in do_mlock overflows to zero due to the following code in mlock:
> 
>    len = PAGE_ALIGN(len + (offset_in_page(start)));
> 
> The same problem happens in munlock.
> 
> Add new check and return -EINVAL to fix this overflowing scenarios since
> they are absolutely wrong.
> 
> Similar logic is used to fix problems with multiple syscalls.
> 
> Changelog since v2[2]:
> - modified the way of checking overflows based on Andrew's comments
> 
> Changelog since v1[1]:
> - only check overflow rather than access_ok to keep backward-compatibility

Do you have some test cases and could we add them to LTP, for example?

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 0/4] Add overflow checks for several syscalls

[mawupeng]

On 2023/2/4 1:10, David Hildenbrand wrote:
> On 28.01.23 07:32, Wupeng Ma wrote:
>> From: Ma Wupeng <mawupeng1@huawei.com>
>>
>> While testing mlock, we have a problem if the len of mlock is ULONG_MAX.
>> The return value of mlock is zero. But nothing will be locked since the
>> len in do_mlock overflows to zero due to the following code in mlock:
>>
>>    len = PAGE_ALIGN(len + (offset_in_page(start)));
>>
>> The same problem happens in munlock.
>>
>> Add new check and return -EINVAL to fix this overflowing scenarios since
>> they are absolutely wrong.
>>
>> Similar logic is used to fix problems with multiple syscalls.
>>
>> Changelog since v2[2]:
>> - modified the way of checking overflows based on Andrew's comments
>>
>> Changelog since v1[1]:
>> - only check overflow rather than access_ok to keep backward-compatibility
> 
> Do you have some test cases and could we add them to LTP, for example?

The basic idea is setting param len to ULONG_MAX. while will overflow if PAGE_ALIGN(len).

Here are some simple examples:

mlock(addr, ULONG_MAX);
set_mempolicy_home_node(addr, ULONG_MAX, 0, 0);
mbind(addr, ULONG_MAX, MPOL_BIND, &nodemask, max_node, MPOL_MF_MOVE_ALL);
msync(addr, ULONG_MAX, MS_ASYNC);

>