[PATCH] mm/migrate: fix do_pages_move for compat pointers

[PATCH] mm/migrate: fix do_pages_move for compat pointers

[Gregory Price]

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

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;
 
-- 
2.39.1

[PATCH 2/2] mm/migrate: remove unused mm argument from do_move_pages_to_node

[Gregory Price]

This function does not actively use the mm_struct, it can be removed.

Signed-off-by: Gregory Price <gregory.price@memverge.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 mm/migrate.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 06086dc9da28..4e8368408b17 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2029,8 +2029,7 @@ static int store_status(int __user *status, int start, int value, int nr)
 	return 0;
 }
 
-static int do_move_pages_to_node(struct mm_struct *mm,
-		struct list_head *pagelist, int node)
+static int do_move_pages_to_node(struct list_head *pagelist, int node)
 {
 	int err;
 	struct migration_target_control mtc = {
@@ -2126,7 +2125,7 @@ static int add_page_for_migration(struct mm_struct *mm, const void __user *p,
 	return err;
 }
 
-static int move_pages_and_store_status(struct mm_struct *mm, int node,
+static int move_pages_and_store_status(int node,
 		struct list_head *pagelist, int __user *status,
 		int start, int i, unsigned long nr_pages)
 {
@@ -2135,7 +2134,7 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node,
 	if (list_empty(pagelist))
 		return 0;
 
-	err = do_move_pages_to_node(mm, pagelist, node);
+	err = do_move_pages_to_node(pagelist, node);
 	if (err) {
 		/*
 		 * Positive err means the number of failed
@@ -2203,7 +2202,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 			current_node = node;
 			start = i;
 		} else if (node != current_node) {
-			err = move_pages_and_store_status(mm, current_node,
+			err = move_pages_and_store_status(current_node,
 					&pagelist, status, start, i, nr_pages);
 			if (err)
 				goto out;
@@ -2238,7 +2237,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		if (err)
 			goto out_flush;
 
-		err = move_pages_and_store_status(mm, current_node, &pagelist,
+		err = move_pages_and_store_status(current_node, &pagelist,
 				status, start, i, nr_pages);
 		if (err) {
 			/* We have accounted for page i */
@@ -2250,7 +2249,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 	}
 out_flush:
 	/* Make sure we do not overwrite the existing error */
-	err1 = move_pages_and_store_status(mm, current_node, &pagelist,
+	err1 = move_pages_and_store_status(current_node, &pagelist,
 				status, start, i, nr_pages);
 	if (err >= 0)
 		err = err1;
-- 
2.39.1

Re: [PATCH] mm/migrate: fix do_pages_move for compat pointers

[Andrew Morton]

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?

Re: [PATCH] mm/migrate: fix do_pages_move for compat pointers

[Arnd Bergmann]

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

Re: [PATCH] mm/migrate: fix do_pages_move for compat pointers

[Arnd Bergmann]

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

Re: [PATCH] mm/migrate: fix do_pages_move for compat pointers

[Andrew Morton]

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?

Re: [PATCH] mm/migrate: fix do_pages_move for compat pointers

[Arnd Bergmann]

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