[PATCH 0/4] return EINVAL for illegal user memory range

[PATCH 0/4] return EINVAL for illegal user memory range

[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)));

However this problem appear in multiple syscalls.

Since TASK_SIZE is the maximum user space address. The start or len of
mlock shouldn't be bigger than this. Function access_ok can be used to
check this issue, so return -EINVAL if bigger.

Ma Wupeng (4):
  mm/mlock: return EINVAL for illegal user memory range in mlock
  mm/mempolicy: return EINVAL for illegal user memory range for
    set_mempolicy_home_node
  mm/mempolicy: return EINVAL for illegal user memory range for mbind
  mm/msync: return EINVAL for illegal user memory range for msync

 mm/mempolicy.c | 7 +++++++
 mm/mlock.c     | 6 ++++++
 mm/msync.c     | 2 ++
 3 files changed, 15 insertions(+)

-- 
2.25.1

[PATCH 1/4] mm/mlock: return EINVAL for illegal user memory range in mlock

[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.

Since TASK_SIZE is the maximum user space address. The start or len of
mlock shouldn't be bigger than this. Function access_ok can be used to
check this issue, so return -EINVAL if bigger.

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

diff --git a/mm/mlock.c b/mm/mlock.c
index 7032f6dd0ce1..b9422a62a4cf 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -575,6 +575,9 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
 	if (!can_do_mlock())
 		return -EPERM;
 
+	if (unlikely(!access_ok((void __user *)start, len)))
+		return -EINVAL;
+
 	len = PAGE_ALIGN(len + (offset_in_page(start)));
 	start &= PAGE_MASK;
 
@@ -635,6 +638,9 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
 
 	start = untagged_addr(start);
 
+	if (unlikely(!access_ok((void __user *)start, len)))
+		return -EINVAL;
+
 	len = PAGE_ALIGN(len + (offset_in_page(start)));
 	start &= PAGE_MASK;
 
-- 
2.25.1

Re: [PATCH 1/4] mm/mlock: return EINVAL for illegal user memory range in mlock

[mawupeng]

On 2022/12/5 11:41, 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.
> 
> Since TASK_SIZE is the maximum user space address. The start or len of
> mlock shouldn't be bigger than this. Function access_ok can be used to
> check this issue, so return -EINVAL if bigger.
> 
> Signed-off-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>  mm/mlock.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 7032f6dd0ce1..b9422a62a4cf 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -575,6 +575,9 @@ static __must_check int do_mlock(unsigned long start, size_t len, vm_flags_t fla
>  	if (!can_do_mlock())
>  		return -EPERM;
>  
> +	if (unlikely(!access_ok((void __user *)start, len)))
> +		return -EINVAL;

When we are runing ltp testcase, a error occurs on mlock[1]. It seems that
ENOMEN is expencted for this testcase.

In the manual of mlock[2]

       ENOMEM (mlock(), mlock2(), and munlock()) Some of the specified
              address range does not correspond to mapped pages in the
              address space of the process.

ENOMEM seem more appropriate for this situation?

[1] https://github.com/linux-test-project/ltp/blob/20220930/testcases/open_posix_testsuite/conformance/interfaces/mlock/8-1.c
[2] https://man7.org/linux/man-pages/man2/mlock.2.html

> +
>  	len = PAGE_ALIGN(len + (offset_in_page(start)));
>  	start &= PAGE_MASK;
>  
> @@ -635,6 +638,9 @@ SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len)
>  
>  	start = untagged_addr(start);
>  
> +	if (unlikely(!access_ok((void __user *)start, len)))
> +		return -EINVAL;
> +
>  	len = PAGE_ALIGN(len + (offset_in_page(start)));
>  	start &= PAGE_MASK;
>  

Re: [PATCH 1/4] mm/mlock: return EINVAL for illegal user memory range in mlock

[Andrew Morton]

On Mon, 5 Dec 2022 11:41:05 +0800 Wupeng Ma <mawupeng1@huawei.com> wrote:

> 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.
> 
> Since TASK_SIZE is the maximum user space address. The start or len of
> mlock shouldn't be bigger than this. Function access_ok can be used to
> check this issue, so return -EINVAL if bigger.

What happens if userspace uses a value somewhat smaller than ULONG_MAX?

	mlock(addr, ULONG_MAX - 1000000);

?

Because if the above works successfully and if it no longer works
successfully with this patchset then that could be a
backward-compatibility problem.

Re: [PATCH 1/4] mm/mlock: return EINVAL for illegal user memory range in mlock

[mawupeng]

On 2022/12/29 6:17, Andrew Morton wrote:
> On Mon, 5 Dec 2022 11:41:05 +0800 Wupeng Ma <mawupeng1@huawei.com> wrote:
> 
>> 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.
>>
>> Since TASK_SIZE is the maximum user space address. The start or len of
>> mlock shouldn't be bigger than this. Function access_ok can be used to
>> check this issue, so return -EINVAL if bigger.
> 
> What happens if userspace uses a value somewhat smaller than ULONG_MAX?
> 
> 	mlock(addr, ULONG_MAX - 1000000);
> 
> ?
> 
> Because if the above works successfully and if it no longer works
> successfully with this patchset then that could be a
> backward-compatibility problem.

For mlock:

 mlock(addr, ULONG_MAX - 1000000) will ret -1 and the errno is EINVAL(22) due to
the following call trace.

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

Just like you said, we need to keep backward-compatibility. Maybe we can only catch and fix
the overflowing scenarios since they are absolutely wrong. here is the diff:

diff --git a/mm/mlock.c b/mm/mlock.c
index 7032f6dd0ce1..fd5e857ab245 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -569,6 +569,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);
 
@@ -578,6 +579,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;


> 

[PATCH 2/4] mm/mempolicy: return EINVAL for illegal user memory range for set_mempolicy_home_node

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

Add access_ok to check user memory range and return EINVAL if overflows.

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

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 61aa9aedb728..e3a2c465fe8a 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1499,6 +1499,10 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
 	start = untagged_addr(start);
 	if (start & ~PAGE_MASK)
 		return -EINVAL;
+
+	if (unlikely(!access_ok((void __user *)start, len)))
+		return -EINVAL;
+
 	/*
 	 * flags is used for future extension if any.
 	 */
-- 
2.25.1

[PATCH 3/4] mm/mempolicy: return EINVAL for illegal user memory range for mbind

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

Add access_ok to check user memory range and return EINVAL if overflows for
mbind.

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

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index e3a2c465fe8a..a6bddf454953 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1272,6 +1272,9 @@ static long do_mbind(unsigned long start, unsigned long len,
 	if (start & ~PAGE_MASK)
 		return -EINVAL;
 
+	if (unlikely(!access_ok((void __user *)start, len)))
+		return -EINVAL;
+
 	if (mode == MPOL_DEFAULT)
 		flags &= ~MPOL_MF_STRICT;
 
-- 
2.25.1

[PATCH 4/4] mm/msync: return EINVAL for illegal user memory range for msync

[Wupeng Ma]

From: Ma Wupeng <mawupeng1@huawei.com>

Add access_ok to check user memory range and return EINVAL if overflows for
msync.

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

diff --git a/mm/msync.c b/mm/msync.c
index ac4c9bfea2e7..a87c3dca473a 100644
--- a/mm/msync.c
+++ b/mm/msync.c
@@ -43,6 +43,8 @@ SYSCALL_DEFINE3(msync, unsigned long, start, size_t, len, int, flags)
 		goto out;
 	if (offset_in_page(start))
 		goto out;
+	if (unlikely(!access_ok((void __user *)start, len)))
+		goto out;
 	if ((flags & MS_ASYNC) && (flags & MS_SYNC))
 		goto out;
 	error = -ENOMEM;
-- 
2.25.1

Re: [PATCH 0/4] return EINVAL for illegal user memory range

[mawupeng]

Hi, maintainers, kindly ping...

Thanks.
Ma.

Re: [PATCH 0/4] return EINVAL for illegal user memory range

[David Hildenbrand]

On 05.12.22 04:41, 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)));
> 
> However this problem appear in multiple syscalls.
> 
> Since TASK_SIZE is the maximum user space address. The start or len of
> mlock shouldn't be bigger than this. Function access_ok can be used to
> check this issue, so return -EINVAL if bigger.

I assume this makes sure that what we document holds:

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

So instead of adding access_ok() checks, wouldn't be the right think to 
do checking for overflows?


-- 
Thanks,

David / dhildenb

Re: [PATCH 0/4] return EINVAL for illegal user memory range

[mawupeng]

On 2023/1/2 21:22, David Hildenbrand wrote:
> On 05.12.22 04:41, 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)));
>>
>> However this problem appear in multiple syscalls.
>>
>> Since TASK_SIZE is the maximum user space address. The start or len of
>> mlock shouldn't be bigger than this. Function access_ok can be used to
>> check this issue, so return -EINVAL if bigger.
> 
> I assume this makes sure that what we document holds:
> 
> 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).
> 
> So instead of adding access_ok() checks, wouldn't be the right think to do checking for overflows?

I agree with you. Do checking only for overflows do seems nice since we need to keep
backward-compatibility.

> 
>