| Message ID | 20230116115813.2956935-2-mawupeng1@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add overflow checks for several syscalls | expand |
On Mon, 16 Jan 2023 19:58:10 +0800 Wupeng Ma <mawupeng1@huawei.com> 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. > > ... > > --- 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; I'm not sure that "old_len" is a good identifier. It reads to me like "the length of the old mlocked region" or something. I really don't like it when functions modify the values of the incoming argument like this. It would be better to leave `len' alone and create a new_len or something. > 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; It would be clearer to do this immediately after calculating the new value of `len'. Before going on to play with `start'. Can we do something like this? --- a/mm/mlock.c~a +++ a/mm/mlock.c @@ -575,7 +575,12 @@ static __must_check int do_mlock(unsigne if (!can_do_mlock()) return -EPERM; - len = PAGE_ALIGN(len + (offset_in_page(start))); + if (len) { + len = PAGE_ALIGN(len + (offset_in_page(start))); + if (len == 0) /* overflow */ + return -EINVAL; + } + start &= PAGE_MASK; lock_limit = rlimit(RLIMIT_MEMLOCK);
On 2023/1/17 4:51, Andrew Morton wrote: > On Mon, 16 Jan 2023 19:58:10 +0800 Wupeng Ma <mawupeng1@huawei.com> 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. >> >> ... >> >> --- 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; > > I'm not sure that "old_len" is a good identifier. It reads to me like > "the length of the old mlocked region" or something. > > I really don't like it when functions modify the values of the incoming > argument like this. It would be better to leave `len' alone and create > a new_len or something. Thanks for your reviewing. You do have a point in saying that. > >> 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; > > It would be clearer to do this immediately after calculating the new > value of `len'. Before going on to play with `start'. > > Can we do something like this? > > --- a/mm/mlock.c~a > +++ a/mm/mlock.c > @@ -575,7 +575,12 @@ static __must_check int do_mlock(unsigne > if (!can_do_mlock()) > return -EPERM; > > - len = PAGE_ALIGN(len + (offset_in_page(start))); > + if (len) { > + len = PAGE_ALIGN(len + (offset_in_page(start))); > + if (len == 0) /* overflow */ > + return -EINVAL; > + } > + > start &= PAGE_MASK; > > lock_limit = rlimit(RLIMIT_MEMLOCK); > _ It's really more appropriate to check like this, I will use this in the next patchset. > > That depends on how we handle len==0. afaict, mlock(len==0) will > presently burn a bunch of cpu cycles (not that we want to optimize this > case), do nothing then return 0? We can add and a new check in if len == 0, since the similar check appears in mbind, set_mempolicy_home_node, msync. The origin len == 0 check for mlock/munlock can be found in apply_vma_lock_flags, We can move this check to here to avoid burn a bunch of cpu cycles. do_mlock apply_vma_lock_flags end = start + len; if (end == start) return 0; Can we do something like this? diff --git a/mm/mlock.c b/mm/mlock.c index 7032f6dd0ce1..50a33abc1a2e 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,12 @@ 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 == 0) + return -EINVAL; + start &= PAGE_MASK; lock_limit = rlimit(RLIMIT_MEMLOCK); @@ -632,10 +635,14 @@ SYSCALL_DEFINE3(mlock2, unsigned long, start, size_t, len, int, flags) SYSCALL_DEFINE2(munlock, unsigned long, start, size_t, len) { int ret; - start = untagged_addr(start); + if (!len) + return 0; len = PAGE_ALIGN(len + (offset_in_page(start))); + if (len == 0) + return -EINVAL; + start &= PAGE_MASK; if (mmap_write_lock_killable(current->mm)) >
diff --git a/mm/mlock.c b/mm/mlock.c index 7032f6dd0ce1..5a4e767feb28 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; @@ -632,12 +636,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);