| Message ID | 20230320024739.224850-2-mawupeng1@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add overflow checks for several syscalls | expand |
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);
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); >
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); >
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.
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. >
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.
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.
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? >
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.
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);