linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/4] Add overflow checks for several syscalls
@ 2023-03-20  2:47 Wupeng Ma
  2023-03-20  2:47 ` [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock Wupeng Ma
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Wupeng Ma @ 2023-03-20  2:47 UTC (permalink / raw)
  To: akpm; +Cc: linux-mm, linux-kernel, mawupeng1, kuleshovmail, aneesh.kumar, david

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.

Here is the testcases:

  #include <stdio.h>
  #define _GNU_SOURCE
  #include <sys/mman.h>
  #include <fcntl.h>
  #include <errno.h>
  #include <unistd.h>
  #include <string.h>
  #include <limits.h>
  #include <numaif.h>
  
  extern int errno;
  int main(void)
  {
      int fd;
      int ret;
      void *addr;
      int size = getpagesize();
  
      fd = open("/tmp/testfile", O_RDWR | O_CREAT);
      if (fd < 0) {
          printf("open file error! errno: %d\n", errno);
          return -1;
      }
      printf("open success\n");
      addr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
      if (addr == MAP_FAILED) {
          printf("open file error! errno: %d\n", errno);
          close(fd);
          return -1;
      }
      printf("mmap success\n");
      memset(addr, 0, size);
  
      printf("==== mlock ====\n");
      ret = mlock(addr, ULONG_MAX);
      if (ret == -1 && errno == EINVAL)
          printf("mlock test passed\n");
      else
          printf("mlock test failed, ret: %d, errno: %d\n", ret, errno);
  
      printf("==== set_mempolicy_home_node ====\n");
      ret = syscall(450, addr, ULONG_MAX, 0, 0);
      if (ret == -1 && errno == EINVAL)
          printf("set_mempolicy_home_node test passed\n");
      else
          printf("set_mempolicy_home_node test failed, ret: %d, errno: %d\n", ret, errno);
  
      printf("==== mbind ====\n");
      unsigned long nodemask = 1Ul << 0;
      long max_node = 8 * sizeof(nodemask);
      ret = mbind(addr, ULONG_MAX, MPOL_BIND, &nodemask, max_node, MPOL_MF_MOVE_ALL);
      if (ret == -1 && errno == EINVAL)
          printf("mbind test passed\n");
      else
          printf("mbind test failed, ret: %d, errno: %d\n", ret, errno);
  
      printf("==== msync ====\n");
      ret = msync(addr, ULONG_MAX, MS_ASYNC);
      if (ret == -1 && errno == ENOMEM)
          printf("mbind test passed\n");
      else
          printf("mbind test failed, ret: %d, errno: %d\n", ret, errno);
  
      munmap(mmap, size);
  
      return 0;
  }

Changelog since v3[3]:
- rebase to the latest master
- add simple testcases

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/
[3]: https://lore.kernel.org/linux-mm/de4149c7-0e6e-2035-3fb8-2f9da9633704@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 | 6 ++++--
 mm/mlock.c     | 8 ++++++++
 mm/msync.c     | 3 ++-
 3 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-20  2:47 [PATCH v4 0/4] Add overflow checks for several syscalls Wupeng Ma
@ 2023-03-20  2:47 ` Wupeng Ma
  2023-03-20 10:54   ` David Hildenbrand
  2023-03-20  2:47 ` [PATCH v4 2/4] mm/mempolicy: return EINVAL for if len overflows for set_mempolicy_home_node Wupeng Ma
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Wupeng Ma @ 2023-03-20  2:47 UTC (permalink / raw)
  To: akpm; +Cc: linux-mm, linux-kernel, mawupeng1, kuleshovmail, aneesh.kumar, david

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.

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

diff --git a/mm/mlock.c b/mm/mlock.c
index 617469fce96d..eb68476da497 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -568,6 +568,7 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
 	unsigned long locked;
 	unsigned long lock_limit;
 	int error = -ENOMEM;
+	size_t old_len = len;
 
 	start = untagged_addr(start);
 
@@ -577,6 +578,9 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
 	len = PAGE_ALIGN(len + (offset_in_page(start)));
 	start &= PAGE_MASK;
 
+	if (old_len != 0 && len == 0)
+		return -EINVAL;
+
 	lock_limit = rlimit(RLIMIT_MEMLOCK);
 	lock_limit >>= PAGE_SHIFT;
 	locked = len >> PAGE_SHIFT;
@@ -631,12 +635,16 @@ SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
 SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
 {
 	int ret;
+	size_t old_len = len;
 
 	start = untagged_addr(start);
 
 	len = PAGE_ALIGN(len + (offset_in_page(start)));
 	start &= PAGE_MASK;
 
+	if (old_len != 0 && len == 0)
+		return -EINVAL;
+
 	if (mmap_write_lock_killable(current->mm))
 		return -EINTR;
 	ret = apply_vma_lock_flags(start, len, 0);
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 2/4] mm/mempolicy: return EINVAL for if len overflows for set_mempolicy_home_node
  2023-03-20  2:47 [PATCH v4 0/4] Add overflow checks for several syscalls Wupeng Ma
  2023-03-20  2:47 ` [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock Wupeng Ma
@ 2023-03-20  2:47 ` Wupeng Ma
  2023-03-20  2:47 ` [PATCH v4 3/4] mm/mempolicy: return EINVAL if len overflows for mbind Wupeng Ma
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Wupeng Ma @ 2023-03-20  2:47 UTC (permalink / raw)
  To: akpm; +Cc: linux-mm, linux-kernel, mawupeng1, kuleshovmail, aneesh.kumar, david

From: Ma Wupeng <mawupeng1@huawei.com>

Return -EINVAL if len overflows for set_mempolicy_home_node.

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

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index a256a241fd1d..3a68998adc3a 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1489,6 +1489,7 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
 		unsigned long, home_node, unsigned long, flags)
 {
 	struct mm_struct *mm = current->mm;
+	unsigned long old_len = len;
 	struct vm_area_struct *vma;
 	struct mempolicy *new, *old;
 	unsigned long vmstart;
@@ -1516,7 +1517,7 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
 	len = PAGE_ALIGN(len);
 	end = start + len;
 
-	if (end < start)
+	if (end < start || (old_len != 0 && len == 0))
 		return -EINVAL;
 	if (end == start)
 		return 0;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 3/4] mm/mempolicy: return EINVAL if len overflows for mbind
  2023-03-20  2:47 [PATCH v4 0/4] Add overflow checks for several syscalls Wupeng Ma
  2023-03-20  2:47 ` [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock Wupeng Ma
  2023-03-20  2:47 ` [PATCH v4 2/4] mm/mempolicy: return EINVAL for if len overflows for set_mempolicy_home_node Wupeng Ma
@ 2023-03-20  2:47 ` Wupeng Ma
  2023-03-20  2:47 ` [PATCH v4 4/4] mm/msync: return ENOMEM if len overflows for msync Wupeng Ma
  2023-03-20 11:06 ` [PATCH v4 0/4] Add overflow checks for several syscalls mawupeng
  4 siblings, 0 replies; 15+ messages in thread
From: Wupeng Ma @ 2023-03-20  2:47 UTC (permalink / raw)
  To: akpm; +Cc: linux-mm, linux-kernel, mawupeng1, kuleshovmail, aneesh.kumar, david

From: Ma Wupeng <mawupeng1@huawei.com>

Return -EINVAL if len overflows for mbind.

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

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 3a68998adc3a..6b1c45021e48 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1259,6 +1259,7 @@ static long do_mbind(unsigned long start, unsigned long len,
 		     nodemask_t *nmask, unsigned long flags)
 {
 	struct mm_struct *mm = current->mm;
+	unsigned long old_len = len;
 	struct mempolicy *new;
 	unsigned long end;
 	int err;
@@ -1279,7 +1280,7 @@ static long do_mbind(unsigned long start, unsigned long len,
 	len = PAGE_ALIGN(len);
 	end = start + len;
 
-	if (end < start)
+	if (end < start || (old_len != 0 && len == 0))
 		return -EINVAL;
 	if (end == start)
 		return 0;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 4/4] mm/msync: return ENOMEM if len overflows for msync
  2023-03-20  2:47 [PATCH v4 0/4] Add overflow checks for several syscalls Wupeng Ma
                   ` (2 preceding siblings ...)
  2023-03-20  2:47 ` [PATCH v4 3/4] mm/mempolicy: return EINVAL if len overflows for mbind Wupeng Ma
@ 2023-03-20  2:47 ` Wupeng Ma
  2023-03-20 11:06 ` [PATCH v4 0/4] Add overflow checks for several syscalls mawupeng
  4 siblings, 0 replies; 15+ messages in thread
From: Wupeng Ma @ 2023-03-20  2:47 UTC (permalink / raw)
  To: akpm; +Cc: linux-mm, linux-kernel, mawupeng1, kuleshovmail, aneesh.kumar, david

From: Ma Wupeng <mawupeng1@huawei.com>

Return -ENOMEM if len overflows for msync.

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

diff --git a/mm/msync.c b/mm/msync.c
index ac4c9bfea2e7..8ac227ec38af 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -36,6 +36,7 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
 	struct vm_area_struct *vma;
 	int unmapped_error = 0;
 	int error = -EINVAL;
+	size_t old_len = len;
 
 	start = untagged_addr(start);
 
@@ -48,7 +49,7 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
 	error = -ENOMEM;
 	len = (len + ~PAGE_MASK) & PAGE_MASK;
 	end = start + len;
-	if (end < start)
+	if (end < start || (old_len != 0 && len == 0))
 		goto out;
 	error = 0;
 	if (end == start)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-20  2:47 ` [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock Wupeng Ma
@ 2023-03-20 10:54   ` David Hildenbrand
  2023-03-20 11:05     ` mawupeng
  2023-03-21  7:44     ` mawupeng
  0 siblings, 2 replies; 15+ messages in thread
From: David Hildenbrand @ 2023-03-20 10:54 UTC (permalink / raw)
  To: Wupeng Ma, akpm; +Cc: linux-mm, linux-kernel, kuleshovmail, aneesh.kumar

On 20.03.23 03:47, 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.

Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?

> 
> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>   mm/mlock.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 617469fce96d..eb68476da497 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -568,6 +568,7 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>   	unsigned long locked;
>   	unsigned long lock_limit;
>   	int error = -ENOMEM;
> +	size_t old_len = len;
>   
>   	start = untagged_addr(start);
>   
> @@ -577,6 +578,9 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>   	len = PAGE_ALIGN(len + (offset_in_page(start)));
>   	start &= PAGE_MASK;
>   
> +	if (old_len != 0 && len == 0)

if (old_len && !len)

> +		return -EINVAL;
> +
>   	lock_limit = rlimit(RLIMIT_MEMLOCK);
>   	lock_limit >>= PAGE_SHIFT;
>   	locked = len >> PAGE_SHIFT;
> @@ -631,12 +635,16 @@ SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
>   SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
>   {
>   	int ret;
> +	size_t old_len = len;
>   
>   	start = untagged_addr(start);
>   
>   	len = PAGE_ALIGN(len + (offset_in_page(start)));
>   	start &= PAGE_MASK;
>   
> +	if (old_len != 0 && len == 0)

if (old_len && !len)

> +		return -EINVAL;
> +
>   	if (mmap_write_lock_killable(current->mm))
>   		return -EINTR;
>   	ret = apply_vma_lock_flags(start, len, 0);

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-20 10:54   ` David Hildenbrand
@ 2023-03-20 11:05     ` mawupeng
  2023-03-21  7:44     ` mawupeng
  1 sibling, 0 replies; 15+ messages in thread
From: mawupeng @ 2023-03-20 11:05 UTC (permalink / raw)
  To: david, akpm; +Cc: mawupeng1, linux-mm, linux-kernel, kuleshovmail, aneesh.kumar



On 2023/3/20 18:54, David Hildenbrand wrote:
> On 20.03.23 03:47, 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.
> 
> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?

Thanks for reviewing.

I will test this and resend & reply this.

> 
>>
>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   mm/mlock.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 617469fce96d..eb68476da497 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -568,6 +568,7 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>       unsigned long locked;
>>       unsigned long lock_limit;
>>       int error = -ENOMEM;
>> +    size_t old_len = len;
>>         start = untagged_addr(start);
>>   @@ -577,6 +578,9 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>       len = PAGE_ALIGN(len + (offset_in_page(start)));
>>       start &= PAGE_MASK;
>>   +    if (old_len != 0 && len == 0)
> 
> if (old_len && !len)
> 
>> +        return -EINVAL;
>> +
>>       lock_limit = rlimit(RLIMIT_MEMLOCK);
>>       lock_limit >>= PAGE_SHIFT;
>>       locked = len >> PAGE_SHIFT;
>> @@ -631,12 +635,16 @@ SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
>>   SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
>>   {
>>       int ret;
>> +    size_t old_len = len;
>>         start = untagged_addr(start);
>>         len = PAGE_ALIGN(len + (offset_in_page(start)));
>>       start &= PAGE_MASK;
>>   +    if (old_len != 0 && len == 0)
> 
> if (old_len && !len)

Sorry for wasting your time.

I send the wrong version of this patchset, this is the older version.

> 
>> +        return -EINVAL;
>> +
>>       if (mmap_write_lock_killable(current->mm))
>>           return -EINTR;
>>       ret = apply_vma_lock_flags(start, len, 0);
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 0/4] Add overflow checks for several syscalls
  2023-03-20  2:47 [PATCH v4 0/4] Add overflow checks for several syscalls Wupeng Ma
                   ` (3 preceding siblings ...)
  2023-03-20  2:47 ` [PATCH v4 4/4] mm/msync: return ENOMEM if len overflows for msync Wupeng Ma
@ 2023-03-20 11:06 ` mawupeng
  4 siblings, 0 replies; 15+ messages in thread
From: mawupeng @ 2023-03-20 11:06 UTC (permalink / raw)
  To: akpm; +Cc: mawupeng1, linux-mm, linux-kernel, kuleshovmail, aneesh.kumar, david

Sorry for wasting your time.

I send the wrong version of this patchset, this is the older version.

I will send the right one later.

On 2023/3/20 10:47, 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.
> 
> Here is the testcases:
> 
>   #include <stdio.h>
>   #define _GNU_SOURCE
>   #include <sys/mman.h>
>   #include <fcntl.h>
>   #include <errno.h>
>   #include <unistd.h>
>   #include <string.h>
>   #include <limits.h>
>   #include <numaif.h>
>   
>   extern int errno;
>   int main(void)
>   {
>       int fd;
>       int ret;
>       void *addr;
>       int size = getpagesize();
>   
>       fd = open("/tmp/testfile", O_RDWR | O_CREAT);
>       if (fd < 0) {
>           printf("open file error! errno: %d\n", errno);
>           return -1;
>       }
>       printf("open success\n");
>       addr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>       if (addr == MAP_FAILED) {
>           printf("open file error! errno: %d\n", errno);
>           close(fd);
>           return -1;
>       }
>       printf("mmap success\n");
>       memset(addr, 0, size);
>   
>       printf("==== mlock ====\n");
>       ret = mlock(addr, ULONG_MAX);
>       if (ret == -1 && errno == EINVAL)
>           printf("mlock test passed\n");
>       else
>           printf("mlock test failed, ret: %d, errno: %d\n", ret, errno);
>   
>       printf("==== set_mempolicy_home_node ====\n");
>       ret = syscall(450, addr, ULONG_MAX, 0, 0);
>       if (ret == -1 && errno == EINVAL)
>           printf("set_mempolicy_home_node test passed\n");
>       else
>           printf("set_mempolicy_home_node test failed, ret: %d, errno: %d\n", ret, errno);
>   
>       printf("==== mbind ====\n");
>       unsigned long nodemask = 1Ul << 0;
>       long max_node = 8 * sizeof(nodemask);
>       ret = mbind(addr, ULONG_MAX, MPOL_BIND, &nodemask, max_node, MPOL_MF_MOVE_ALL);
>       if (ret == -1 && errno == EINVAL)
>           printf("mbind test passed\n");
>       else
>           printf("mbind test failed, ret: %d, errno: %d\n", ret, errno);
>   
>       printf("==== msync ====\n");
>       ret = msync(addr, ULONG_MAX, MS_ASYNC);
>       if (ret == -1 && errno == ENOMEM)
>           printf("mbind test passed\n");
>       else
>           printf("mbind test failed, ret: %d, errno: %d\n", ret, errno);
>   
>       munmap(mmap, size);
>   
>       return 0;
>   }
> 
> Changelog since v3[3]:
> - rebase to the latest master
> - add simple testcases
> 
> 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/
> [3]: https://lore.kernel.org/linux-mm/de4149c7-0e6e-2035-3fb8-2f9da9633704@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 | 6 ++++--
>  mm/mlock.c     | 8 ++++++++
>  mm/msync.c     | 3 ++-
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-20 10:54   ` David Hildenbrand
  2023-03-20 11:05     ` mawupeng
@ 2023-03-21  7:44     ` mawupeng
  2023-03-21 14:19       ` David Hildenbrand
  1 sibling, 1 reply; 15+ messages in thread
From: mawupeng @ 2023-03-21  7:44 UTC (permalink / raw)
  To: david, akpm; +Cc: mawupeng1, linux-mm, linux-kernel, kuleshovmail, aneesh.kumar



On 2023/3/20 18:54, David Hildenbrand wrote:
> On 20.03.23 03:47, 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.
> 
> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?

mlock will return 0 if len is zero which is the same w/o this patchset.
Here is the calltrace if len is zero.

mlock(len == 0)
	do_mlock(len == 0)
		if (!len)
			return 0
			
Sorry for wasting your time in the wrong v4. Here is the latest v5:

  https://patchwork.kernel.org/project/linux-mm/list/?series=732216

> 
>>
>> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   mm/mlock.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/mm/mlock.c b/mm/mlock.c
>> index 617469fce96d..eb68476da497 100644
>> --- a/mm/mlock.c
>> +++ b/mm/mlock.c
>> @@ -568,6 +568,7 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>       unsigned long locked;
>>       unsigned long lock_limit;
>>       int error = -ENOMEM;
>> +    size_t old_len = len;
>>         start = untagged_addr(start);
>>   @@ -577,6 +578,9 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>>       len = PAGE_ALIGN(len + (offset_in_page(start)));
>>       start &= PAGE_MASK;
>>   +    if (old_len != 0 && len == 0)
> 
> if (old_len && !len)
> 
>> +        return -EINVAL;
>> +
>>       lock_limit = rlimit(RLIMIT_MEMLOCK);
>>       lock_limit >>= PAGE_SHIFT;
>>       locked = len >> PAGE_SHIFT;
>> @@ -631,12 +635,16 @@ SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags)
>>   SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
>>   {
>>       int ret;
>> +    size_t old_len = len;
>>         start = untagged_addr(start);
>>         len = PAGE_ALIGN(len + (offset_in_page(start)));
>>       start &= PAGE_MASK;
>>   +    if (old_len != 0 && len == 0)
> 
> if (old_len && !len)
> 
>> +        return -EINVAL;
>> +
>>       if (mmap_write_lock_killable(current->mm))
>>           return -EINTR;
>>       ret = apply_vma_lock_flags(start, len, 0);
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-21  7:44     ` mawupeng
@ 2023-03-21 14:19       ` David Hildenbrand
  2023-03-22  2:14         ` mawupeng
  0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2023-03-21 14:19 UTC (permalink / raw)
  To: mawupeng, akpm; +Cc: linux-mm, linux-kernel, kuleshovmail, aneesh.kumar

On 21.03.23 08:44, mawupeng wrote:
> 
> 
> On 2023/3/20 18:54, David Hildenbrand wrote:
>> On 20.03.23 03:47, 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.
>>
>> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?
> 
> mlock will return 0 if len is zero which is the same w/o this patchset.
> Here is the calltrace if len is zero.
> 
> mlock(len == 0)
> 	do_mlock(len == 0)
> 		if (!len)
> 			return 0
>

I was asking about addr=0, len=ULONG_MAX.

IIUC, that used to work but could now fail? I haven't played with it, 
though.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-21 14:19       ` David Hildenbrand
@ 2023-03-22  2:14         ` mawupeng
  2023-03-22  8:54           ` David Hildenbrand
  0 siblings, 1 reply; 15+ messages in thread
From: mawupeng @ 2023-03-22  2:14 UTC (permalink / raw)
  To: david, akpm; +Cc: mawupeng1, linux-mm, linux-kernel, kuleshovmail, aneesh.kumar



On 2023/3/21 22:19, David Hildenbrand wrote:
> On 21.03.23 08:44, mawupeng wrote:
>>
>>
>> On 2023/3/20 18:54, David Hildenbrand wrote:
>>> On 20.03.23 03:47, 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.
>>>
>>> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?
>>
>> mlock will return 0 if len is zero which is the same w/o this patchset.
>> Here is the calltrace if len is zero.
>>
>> mlock(len == 0)
>>     do_mlock(len == 0)
>>         if (!len)
>>             return 0
>>
> 
> I was asking about addr=0, len=ULONG_MAX.
> 
> IIUC, that used to work but could now fail? I haven't played with it, though.

Thanks for reviewing.

Previous for add = 0 and len == ULONG_MAX, mlock will return ok(0) since len overflows to zero.
IFAICT, this is not right since mlock do noting(lock nothing) and return ok(0).

With this patch, for the same situation, mlock can return EINVAL as expected.

> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-22  2:14         ` mawupeng
@ 2023-03-22  8:54           ` David Hildenbrand
  2023-03-22  9:01             ` David Hildenbrand
  0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2023-03-22  8:54 UTC (permalink / raw)
  To: mawupeng, akpm; +Cc: linux-mm, linux-kernel, kuleshovmail, aneesh.kumar

On 22.03.23 03:14, mawupeng wrote:
> 
> 
> On 2023/3/21 22:19, David Hildenbrand wrote:
>> On 21.03.23 08:44, mawupeng wrote:
>>>
>>>
>>> On 2023/3/20 18:54, David Hildenbrand wrote:
>>>> On 20.03.23 03:47, 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.
>>>>
>>>> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?
>>>
>>> mlock will return 0 if len is zero which is the same w/o this patchset.
>>> Here is the calltrace if len is zero.
>>>
>>> mlock(len == 0)
>>>      do_mlock(len == 0)
>>>          if (!len)
>>>              return 0
>>>
>>
>> I was asking about addr=0, len=ULONG_MAX.
>>
>> IIUC, that used to work but could now fail? I haven't played with it, though.
> 
> Thanks for reviewing.
> 
> Previous for add = 0 and len == ULONG_MAX, mlock will return ok(0) since len overflows to zero.
> IFAICT, this is not right since mlock do noting(lock nothing) and return ok(0).
> 
> With this patch, for the same situation, mlock can return EINVAL as expected.

Quoting the man page:

"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)."

ULONG_MAX+0 = ULONG_MAX

There is no overflow expected. The proper way to implement it would be 
to handle that case and not fail with EINVAL.

At least that would be expected when reading the man page.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-22  8:54           ` David Hildenbrand
@ 2023-03-22  9:01             ` David Hildenbrand
  2023-03-22  9:20               ` mawupeng
  0 siblings, 1 reply; 15+ messages in thread
From: David Hildenbrand @ 2023-03-22  9:01 UTC (permalink / raw)
  To: mawupeng, akpm; +Cc: linux-mm, linux-kernel, kuleshovmail, aneesh.kumar

On 22.03.23 09:54, David Hildenbrand wrote:
> On 22.03.23 03:14, mawupeng wrote:
>>
>>
>> On 2023/3/21 22:19, David Hildenbrand wrote:
>>> On 21.03.23 08:44, mawupeng wrote:
>>>>
>>>>
>>>> On 2023/3/20 18:54, David Hildenbrand wrote:
>>>>> On 20.03.23 03:47, 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.
>>>>>
>>>>> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?
>>>>
>>>> mlock will return 0 if len is zero which is the same w/o this patchset.
>>>> Here is the calltrace if len is zero.
>>>>
>>>> mlock(len == 0)
>>>>       do_mlock(len == 0)
>>>>           if (!len)
>>>>               return 0
>>>>
>>>
>>> I was asking about addr=0, len=ULONG_MAX.
>>>
>>> IIUC, that used to work but could now fail? I haven't played with it, though.
>>
>> Thanks for reviewing.
>>
>> Previous for add = 0 and len == ULONG_MAX, mlock will return ok(0) since len overflows to zero.
>> IFAICT, this is not right since mlock do noting(lock nothing) and return ok(0).
>>
>> With this patch, for the same situation, mlock can return EINVAL as expected.
> 
> Quoting the man page:
> 
> "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)."
> 
> ULONG_MAX+0 = ULONG_MAX
> 
> There is no overflow expected. The proper way to implement it would be
> to handle that case and not fail with EINVAL.
> 
> At least that would be expected when reading the man page.
> 

As a side note, I agree that failing with EINVAL is better than doing 
noting (mlocking nothing). But I am not sure what we are expected to do 
in that case ... the man page is a bit vague on that.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-22  9:01             ` David Hildenbrand
@ 2023-03-22  9:20               ` mawupeng
  2023-03-22 11:27                 ` David Hildenbrand
  0 siblings, 1 reply; 15+ messages in thread
From: mawupeng @ 2023-03-22  9:20 UTC (permalink / raw)
  To: david, akpm; +Cc: mawupeng1, linux-mm, linux-kernel, kuleshovmail, aneesh.kumar



On 2023/3/22 17:01, David Hildenbrand wrote:
> On 22.03.23 09:54, David Hildenbrand wrote:
>> On 22.03.23 03:14, mawupeng wrote:
>>>
>>>
>>> On 2023/3/21 22:19, David Hildenbrand wrote:
>>>> On 21.03.23 08:44, mawupeng wrote:
>>>>>
>>>>>
>>>>> On 2023/3/20 18:54, David Hildenbrand wrote:
>>>>>> On 20.03.23 03:47, 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.
>>>>>>
>>>>>> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?
>>>>>
>>>>> mlock will return 0 if len is zero which is the same w/o this patchset.
>>>>> Here is the calltrace if len is zero.
>>>>>
>>>>> mlock(len == 0)
>>>>>       do_mlock(len == 0)
>>>>>           if (!len)
>>>>>               return 0
>>>>>
>>>>
>>>> I was asking about addr=0, len=ULONG_MAX.
>>>>
>>>> IIUC, that used to work but could now fail? I haven't played with it, though.
>>>
>>> Thanks for reviewing.
>>>
>>> Previous for add = 0 and len == ULONG_MAX, mlock will return ok(0) since len overflows to zero.
>>> IFAICT, this is not right since mlock do noting(lock nothing) and return ok(0).
>>>
>>> With this patch, for the same situation, mlock can return EINVAL as expected.
>>
>> Quoting the man page:
>>
>> "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)."
>>
>> ULONG_MAX+0 = ULONG_MAX
>>
>> There is no overflow expected. The proper way to implement it would be
>> to handle that case and not fail with EINVAL.
>>
>> At least that would be expected when reading the man page.
>>
> 
> As a side note, I agree that failing with EINVAL is better than doing noting (mlocking nothing). But I am not sure what we are expected to do in that case ... the man page is a bit vague on that.

Thanks for you reviewing.

Can we try to expand the man page since overflow is considered in man page?

> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock
  2023-03-22  9:20               ` mawupeng
@ 2023-03-22 11:27                 ` David Hildenbrand
  0 siblings, 0 replies; 15+ messages in thread
From: David Hildenbrand @ 2023-03-22 11:27 UTC (permalink / raw)
  To: mawupeng, akpm; +Cc: linux-mm, linux-kernel, kuleshovmail, aneesh.kumar

On 22.03.23 10:20, mawupeng wrote:
> 
> 
> On 2023/3/22 17:01, David Hildenbrand wrote:
>> On 22.03.23 09:54, David Hildenbrand wrote:
>>> On 22.03.23 03:14, mawupeng wrote:
>>>>
>>>>
>>>> On 2023/3/21 22:19, David Hildenbrand wrote:
>>>>> On 21.03.23 08:44, mawupeng wrote:
>>>>>>
>>>>>>
>>>>>> On 2023/3/20 18:54, David Hildenbrand wrote:
>>>>>>> On 20.03.23 03:47, 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.
>>>>>>>
>>>>>>> Thinking again, wouldn't we reject mlock(0, ULONG_MAX) now as well?
>>>>>>
>>>>>> mlock will return 0 if len is zero which is the same w/o this patchset.
>>>>>> Here is the calltrace if len is zero.
>>>>>>
>>>>>> mlock(len == 0)
>>>>>>        do_mlock(len == 0)
>>>>>>            if (!len)
>>>>>>                return 0
>>>>>>
>>>>>
>>>>> I was asking about addr=0, len=ULONG_MAX.
>>>>>
>>>>> IIUC, that used to work but could now fail? I haven't played with it, though.
>>>>
>>>> Thanks for reviewing.
>>>>
>>>> Previous for add = 0 and len == ULONG_MAX, mlock will return ok(0) since len overflows to zero.
>>>> IFAICT, this is not right since mlock do noting(lock nothing) and return ok(0).
>>>>
>>>> With this patch, for the same situation, mlock can return EINVAL as expected.
>>>
>>> Quoting the man page:
>>>
>>> "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)."
>>>
>>> ULONG_MAX+0 = ULONG_MAX
>>>
>>> There is no overflow expected. The proper way to implement it would be
>>> to handle that case and not fail with EINVAL.
>>>
>>> At least that would be expected when reading the man page.
>>>
>>
>> As a side note, I agree that failing with EINVAL is better than doing noting (mlocking nothing). But I am not sure what we are expected to do in that case ... the man page is a bit vague on that.
> 
> Thanks for you reviewing.
> 
> Can we try to expand the man page since overflow is considered in man page?

I guess we could spell out that Linux aligns the length up to the next 
page boundary, and that overflow checks are performed on this aligned 
length.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2023-03-22 11:27 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20  2:47 [PATCH v4 0/4] Add overflow checks for several syscalls Wupeng Ma
2023-03-20  2:47 ` [PATCH v4 1/4] mm/mlock: return EINVAL if len overflows for mlock/munlock Wupeng Ma
2023-03-20 10:54   ` David Hildenbrand
2023-03-20 11:05     ` mawupeng
2023-03-21  7:44     ` mawupeng
2023-03-21 14:19       ` David Hildenbrand
2023-03-22  2:14         ` mawupeng
2023-03-22  8:54           ` David Hildenbrand
2023-03-22  9:01             ` David Hildenbrand
2023-03-22  9:20               ` mawupeng
2023-03-22 11:27                 ` David Hildenbrand
2023-03-20  2:47 ` [PATCH v4 2/4] mm/mempolicy: return EINVAL for if len overflows for set_mempolicy_home_node Wupeng Ma
2023-03-20  2:47 ` [PATCH v4 3/4] mm/mempolicy: return EINVAL if len overflows for mbind Wupeng Ma
2023-03-20  2:47 ` [PATCH v4 4/4] mm/msync: return ENOMEM if len overflows for msync Wupeng Ma
2023-03-20 11:06 ` [PATCH v4 0/4] Add overflow checks for several syscalls mawupeng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).