| Message ID | 20231003144857.752952-1-gregory.price@memverge.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/migrate: fix do_pages_move for compat pointers | expand |
On Tue, 3 Oct 2023 10:48:56 -0400 Gregory Price <gourry.memverge@gmail.com> wrote: > do_pages_move does not handle compat pointers for the page list. > correctly. Add in_compat_syscall check and appropriate get_user > fetch when iterating the page list. What are the userspace visible effects of this change?
On Tue, Oct 3, 2023, at 18:49, Andrew Morton wrote: > On Tue, 3 Oct 2023 10:48:56 -0400 Gregory Price > <gourry.memverge@gmail.com> wrote: > >> do_pages_move does not handle compat pointers for the page list. >> correctly. Add in_compat_syscall check and appropriate get_user >> fetch when iterating the page list. > > What are the userspace visible effects of this change? It makes the syscall in compat mode (32-bit userspace, 64-bit kernel) work the same way as the native 32-bit syscall again, restoring the behavior before my broken commit 5b1b561ba73c ("mm: simplify compat_sys_move_pages"). Arnd
On Tue, Oct 3, 2023, at 18:57, Arnd Bergmann wrote: > On Tue, Oct 3, 2023, at 18:49, Andrew Morton wrote: >> On Tue, 3 Oct 2023 10:48:56 -0400 Gregory Price >> <gourry.memverge@gmail.com> wrote: >> >>> do_pages_move does not handle compat pointers for the page list. >>> correctly. Add in_compat_syscall check and appropriate get_user >>> fetch when iterating the page list. >> >> What are the userspace visible effects of this change? > > It makes the syscall in compat mode (32-bit userspace, 64-bit kernel) > work the same way as the native 32-bit syscall again, restoring the > behavior before my broken commit 5b1b561ba73c ("mm: simplify > compat_sys_move_pages"). More specifically, my patch moved the parsing of the 'pages' array from the main entry point into do_pages_stat(), which left the syscall working correctly for the 'stat' operation (nodes = NULL), while the 'move' operation (nodes != NULL) is now missing the conversion and interprets 'pages' as an array of 64-bit pointers instead of the intended 32-bit userspace pointers. Arnd
On Tue, 03 Oct 2023 19:01:45 +0200 "Arnd Bergmann" <arnd@arndb.de> wrote: > On Tue, Oct 3, 2023, at 18:57, Arnd Bergmann wrote: > > On Tue, Oct 3, 2023, at 18:49, Andrew Morton wrote: > >> On Tue, 3 Oct 2023 10:48:56 -0400 Gregory Price > >> <gourry.memverge@gmail.com> wrote: > >> > >>> do_pages_move does not handle compat pointers for the page list. > >>> correctly. Add in_compat_syscall check and appropriate get_user > >>> fetch when iterating the page list. > >> > >> What are the userspace visible effects of this change? > > > > It makes the syscall in compat mode (32-bit userspace, 64-bit kernel) > > work the same way as the native 32-bit syscall again, restoring the > > behavior before my broken commit 5b1b561ba73c ("mm: simplify > > compat_sys_move_pages"). > > More specifically, my patch moved the parsing of the 'pages' > array from the main entry point into do_pages_stat(), which left > the syscall working correctly for the 'stat' operation (nodes = NULL), > while the 'move' operation (nodes != NULL) is now missing > the conversion and interprets 'pages' as an array of 64-bit > pointers instead of the intended 32-bit userspace pointers. > Thanks. So is a cc:stable warranted?
On Tue, Oct 3, 2023, at 19:17, Andrew Morton wrote: > On Tue, 03 Oct 2023 19:01:45 +0200 "Arnd Bergmann" <arnd@arndb.de> wrote: >> On Tue, Oct 3, 2023, at 18:57, Arnd Bergmann wrote: >> > On Tue, Oct 3, 2023, at 18:49, Andrew Morton wrote: >> >> On Tue, 3 Oct 2023 10:48:56 -0400 Gregory Price >> >> <gourry.memverge@gmail.com> wrote: >> >> >> >>> do_pages_move does not handle compat pointers for the page list. >> >>> correctly. Add in_compat_syscall check and appropriate get_user >> >>> fetch when iterating the page list. >> >> >> >> What are the userspace visible effects of this change? >> > >> > It makes the syscall in compat mode (32-bit userspace, 64-bit kernel) >> > work the same way as the native 32-bit syscall again, restoring the >> > behavior before my broken commit 5b1b561ba73c ("mm: simplify >> > compat_sys_move_pages"). >> >> More specifically, my patch moved the parsing of the 'pages' >> array from the main entry point into do_pages_stat(), which left >> the syscall working correctly for the 'stat' operation (nodes = NULL), >> while the 'move' operation (nodes != NULL) is now missing >> the conversion and interprets 'pages' as an array of 64-bit >> pointers instead of the intended 32-bit userspace pointers. >> > > Thanks. So is a cc:stable warranted? Yes, absolutely. It is possible that nobody noticed this bug because the few applications that actually call move_pages are unlikely to run in compat mode because of their large memory requirements, but this clearly fixes a user-visible regression and should have been caught by ltp. Arnd
diff --git a/mm/migrate.c b/mm/migrate.c index 2053b54556ca..06086dc9da28 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2162,6 +2162,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, const int __user *nodes, int __user *status, int flags) { + compat_uptr_t __user *compat_pages = (void __user *)pages; int current_node = NUMA_NO_NODE; LIST_HEAD(pagelist); int start, i; @@ -2174,8 +2175,17 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, int node; err = -EFAULT; - if (get_user(p, pages + i)) - goto out_flush; + if (in_compat_syscall()) { + compat_uptr_t cp; + + if (get_user(cp, compat_pages + i)) + goto out_flush; + + p = compat_ptr(cp); + } else { + if (get_user(p, pages + i)) + goto out_flush; + } if (get_user(node, nodes + i)) goto out_flush;
do_pages_move does not handle compat pointers for the page list. correctly. Add in_compat_syscall check and appropriate get_user fetch when iterating the page list. Fixes: 5b1b561ba73c ("mm: simplify compat_sys_move_pages") Signed-off-by: Gregory Price <gregory.price@memverge.com> Reported-by: Arnd Bergmann <arnd@arndb.de> Co-developed-by: Arnd Bergmann <arnd@arndb.de> --- mm/migrate.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)