fix move/migrate_pages() race on task struct

sys_move_pages() and sys_migrate_pages() are a pretty nice copy and paste job of each other. They both take a pid, find the task struct, and then grab a ref on the mm. They both also do an rcu_read_unlock() after they've taken the mm and then proceed to access 'task'. I think this is a bug in both cases.

This patch takes the pid-to-task code along with the credential and security checks in sys_move_pages() and sys_migrate_pages() and consolidates them. It now takes a task reference in the new function and requires the caller to drop it. I believe this resolves the race.

I think I got lucky that my task_struct was bogus in the oops below. It's probably quite feasible that a task_struct could get freed back in to the slab, reallocated as another task_struct, and then we do these cred checks against a valid, but basically random task.

This patch takes the pid-to-task code along with the credential and security checks in sys_move_pages() and sys_migrate_pages() and consolidates them. It now takes a task reference in the new function and requires the caller to drop it. I believe this resolves the race.

include/linux/migrate.h renamed from include/linux/migrate.h~movememory-helper +1 -0

+++ linux-2.6.git-dave/mm/mempolicy.c	2012-02-16 09:59:17.286207438 -0800

diff -puN mm/migrate.c~movememory-helper mm/migrate.c

mm/migrate.c renamed from mm/migrate.c~movememory-helper +5 -17

@@ -1339,38 +1339,22 @@ static int do_pages_stat(struct mm_struc
 }
 
 /*
- * Move a list of pages in the address space of the currently executing
- * process.
+ * If successful, takes a task_struct reference that
+ * the caller is responsible for releasing.
  */
-SYSCALL_DEFINE6(move_pages, pid_t, pid, unsigned long, nr_pages,
-		const void __user * __user *, pages,
-		const int __user *, nodes,
-		int __user *, status, int, flags)
+struct task_struct *can_migrate_get_task(pid_t pid)
 {
-	const struct cred *cred = current_cred(), *tcred;
 	struct task_struct *task;
-	struct mm_struct *mm;
-	int err;
-
-	/* Check flags */
-	if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))
-		return -EINVAL;
-
-	if ((flags & MPOL_MF_MOVE_ALL) && !capable(CAP_SYS_NICE))
-		return -EPERM;
+	const struct cred *cred = current_cred(), *tcred;
+	int err = 0;
 
-	/* Find the mm_struct */
 	rcu_read_lock();
 	task = pid ? find_task_by_vpid(pid) : current;
 	if (!task) {

Quoted 5 lines · Dave Hansen

This patch takes the pid-to-task code along with the credential and security checks in sys_move_pages() and sys_migrate_pages() and consolidates them. It now takes a task reference in the new function and requires the caller to drop it. I believe this resolves the race.

And this way its safer?

include/linux/migrate.h extern int migrate_vmas(struct mm_struct · line 34

Quoted 8 lines

diff -puN include/linux/migrate.h~movememory-helper include/linux/migrate.h

include/linux/migrate.h renamed from include/linux/migrate.h~movememory-helper +1 -0

@@ -31,6 +31,7 @@ extern int migrate_vmas(struct mm_struct
 extern void migrate_page_copy(struct page *newpage, struct page *page);
 extern int migrate_huge_page_move_mapping(struct address_space *mapping,
 				  struct page *newpage, struct page *page);
+struct task_struct *can_migrate_get_task(pid_t pid);

Could we use something easier to understand? try_get_task()?

        tcred = __task_cred(task);
        if (cred->euid != tcred->suid && cred->euid != tcred->uid &&
            cred->uid  != tcred->suid && cred->uid  != tcred->uid &&
            !capable(CAP_SYS_NICE)) {
                err = -EPERM;
                goto out;
        }

        err = security_task_movememory(task);

Quoted 26 lines

include/linux/migrate.h renamed from include/linux/migrate.h~movememory-helper +4 -14

+struct task_struct *can_migrate_get_task(pid_t pid)
 {
-	const struct cred *cred = current_cred(), *tcred;
 	struct task_struct *task;
-	struct mm_struct *mm;
-	int err;
-
-	/* Check flags */
-	if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))
-		return -EINVAL;
-
-	if ((flags & MPOL_MF_MOVE_ALL) && !capable(CAP_SYS_NICE))
-		return -EPERM;
+	const struct cred *cred = current_cred(), *tcred;
+	int err = 0;
 
-	/* Find the mm_struct */
 	rcu_read_lock();
 	task = pid ? find_task_by_vpid(pid) : current;
 	if (!task) {
 		rcu_read_unlock();
-		return -ESRCH;
+		return ERR_PTR(-ESRCH);
 	}
-	mm = get_task_mm(task);
-	rcu_read_unlock();

include/linux/migrate.h

Quoted 4 lines

include/linux/migrate.h renamed from include/linux/migrate.h~movememory-helper +1 -3

-
-	if (!mm)
-		return -EINVAL;
+	get_task_struct(task);

Hmmm isnt the race still there between the determination of the task and the get_task_struct()? You would have to verify after the get_task_struct that this is really the task we wanted to avoid the race.

Quoted 3 lines

Hmmm isnt the race still there between the determination of the task and the get_task_struct()? You would have to verify after the get_task_struct that this is really the task we wanted to avoid the race.

It's true that selecting a task by pid is inherently racy. What that code does is ensure that the task you've got current has 'pid', but not ensure that 'pid' has never represented another task. But, that's what we do everywhere else in the kernel; there's not much better that we can do.

Maybe "race" is the wrong word for what we've got here. It's a lack of a refcount being taken.

Quoted 8 lines · Dave Hansen

Quoted 3 lines

Hmmm isnt the race still there between the determination of the task and the get_task_struct()? You would have to verify after the get_task_struct that this is really the task we wanted to avoid the race.

It's true that selecting a task by pid is inherently racy. What that code does is ensure that the task you've got current has 'pid', but not ensure that 'pid' has never represented another task. But, that's what we do everywhere else in the kernel; there's not much better that we can do.

We may at this point be getting a reference to a task struct from another process not only from the current process (where the above procedure is valid). You rightly pointed out that the slab rcu free mechanism allows a free and a reallocation within the RCU period.

The effect is that the task
struct could be pointing to a task with another pid that what we were looking for and therefore migrate_pages could subsequently be operating on a totally different process.

The patch does not fix that race so far.

I think you have to verify that the pid of the task matches after you took the refcount in order to be safe. If it does not match then abort.

Quoted 2 lines

Maybe "race" is the wrong word for what we've got here. It's a lack of a refcount being taken.

Is that a real difference or are you just playing with words?

Quoted 4 lines

We may at this point be getting a reference to a task struct from another process not only from the current process (where the above procedure is valid). You rightly pointed out that the slab rcu free mechanism allows a free and a reallocation within the RCU period.

I didn't _mean_ to point that out, but I think I realize what you're talking about. What we have before this patch is this:

        rcu_read_lock();
        task = pid ? find_task_by_vpid(pid) : current;

        rcu_read_unlock();

Quoted 1 line

Is that a real difference or are you just playing with words?

I think we're talking about two different things:
1. does RCU protect the pid->task lookup sufficiently?

2. Can the task simply go away in the move/migrate_pages() calls?

Quoted 10 lines · Dave Hansen

Quoted 4 lines

We may at this point be getting a reference to a task struct from another process not only from the current process (where the above procedure is valid). You rightly pointed out that the slab rcu free mechanism allows a free and a reallocation within the RCU period.

I didn't _mean_ to point that out, but I think I realize what you're talking about. What we have before this patch is this:

        rcu_read_lock();
        task = pid ? find_task_by_vpid(pid) : current;

We take a refcount here on the mm ... See the code. We could simply take a refcount on the task as well if this is considered safe enough. If we have a refcount on the task then we do not need the refcount on the mm. Thats was your approach...

Quoted 6 lines

        rcu_read_unlock();

Quoted 1 line

Is that a real difference or are you just playing with words?

I think we're talking about two different things:
1. does RCU protect the pid->task lookup sufficiently?

I dont know

Quoted 1 line

2. Can the task simply go away in the move/migrate_pages() calls?

The task may go away but we need the mm to stay for migration. That is why a refcount is taken on the mm.

The bug in migrate_pages() is that we do a rcu_unlock and a rcu_lock. If we drop those then we should be safe if the use of a task pointer within a rcu section is safe without taking a refcount.

Quoted 3 lines

The bug in migrate_pages() is that we do a rcu_unlock and a rcu_lock. If we drop those then we should be safe if the use of a task pointer within a rcu section is safe without taking a refcount.

Yes the user of a task_struct pointer found via a userspace pid is valid for the life of an rcu critical section, and the bug is indeed that we drop the rcu_lock and somehow expect the task to remain valid.

The guarantee comes from release_task. In release_task we call __exit_signal which calls __unhash_process, and then we call delayed_put_task to guarantee that the task lives until the end of the rcu interval.

In migrate_pages we have a lot of task accesses outside of the rcu critical section, and without a reference count on task.

I tell you the truth trying to figure out what that code needs to be correct if task != current makes my head hurt.

I think we need to grab a reference on task_struct, to stop the task from going away, and in addition we need to hold task_lock. To keep task->mm from changing (see exec_mmap). But we can't do that and sleep so I think the entire function needs to be rewritten, and the need for task deep in the migrate_pages path needs to be removed as even with the reference count held we can race with someone calling exec.

Quoted 4 lines · Eric W. Biederman

Quoted 3 lines

The bug in migrate_pages() is that we do a rcu_unlock and a rcu_lock. If we drop those then we should be safe if the use of a task pointer within a rcu section is safe without taking a refcount.

Quoted 8 lines

Yes the user of a task_struct pointer found via a userspace pid is valid for the life of an rcu critical section, and the bug is indeed that we drop the rcu_lock and somehow expect the task to remain valid.

The guarantee comes from release_task. In release_task we call __exit_signal which calls __unhash_process, and then we call delayed_put_task to guarantee that the task lives until the end of the rcu interval.

Ah. Ok. Great.

Quoted 2 lines

In migrate_pages we have a lot of task accesses outside of the rcu critical section, and without a reference count on task.

Yes but that is only of interesting for setup and verification of permissions. What matters during migration is that the mm_struct does not go away and we take a refcount on that one.

Quoted 2 lines

I tell you the truth trying to figure out what that code needs to be correct if task != current makes my head hurt.

Hmm...

Quoted 6 lines

I think we need to grab a reference on task_struct, to stop the task from going away, and in addition we need to hold task_lock. To keep task->mm from changing (see exec_mmap). But we can't do that and sleep so I think the entire function needs to be rewritten, and the need for task deep in the migrate_pages path needs to be removed as even with the reference count held we can race with someone calling exec.

We dont need the task during migration. We only need the mm. The task is safe until rcu_read_unlock therefore maybe the following should fix migrate pages:

Subject: migration: Do not do rcu_read_unlock until the last time we need the task_struct pointer

Migration functions perform the rcu_read_unlock too early. As a result the task pointed to may change. Bugs were introduced when adding security checks because rcu_unlock/lock sequences were inserted. Plus the security checks and do_move_pages used the task_struct pointer after rcu_unlock.

Fix those issues by removing the unlock/lock sequences and moving the rcu_read_unlock after the last use of the task struct pointer.

Signed-off-by Christoph Lameter <[email protected]>

---
 mm/mempolicy.c |   22 +++++++++++-----------
 mm/migrate.c   |   28 +++++++++++++++-------------
 2 files changed, 26 insertions(+), 24 deletions(-)

Index: linux-2.6/mm/mempolicy.c
===================================================================

mm/mempolicy.c +11 -11

@@ -1318,16 +1318,14 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	rcu_read_lock();
 	task = pid ? find_task_by_vpid(pid) : current;
 	if (!task) {
-		rcu_read_unlock();
 		err = -ESRCH;
-		goto out;
+		goto unlock_out;
 	}
 	mm = get_task_mm(task);
-	rcu_read_unlock();
 
 	err = -EINVAL;
 	if (!mm)
-		goto out;
+		goto unlock_out;
 
 	/*
 	 * Check if this process has the right to modify the specified

@@ -1335,33 +1333,31 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	 * capabilities, superuser privileges or the same
 	 * userid as the target process.
 	 */
-	rcu_read_lock();
 	tcred = __task_cred(task);
 	if (cred->euid != tcred->suid && cred->euid != tcred->uid &&
 	    cred->uid  != tcred->suid && cred->uid  != tcred->uid &&
 	    !capable(CAP_SYS_NICE)) {
-		rcu_read_unlock();
 		err = -EPERM;
-		goto out;
+		goto unlock_out;
 	}
-	rcu_read_unlock();
 
 	task_nodes = cpuset_mems_allowed(task);
 	/* Is the user allowed to access the target nodes? */
 	if (!nodes_subset(*new, task_nodes) && !capable(CAP_SYS_NICE)) {
 		err = -EPERM;
-		goto out;
+		goto unlock_out;
 	}
 
 	if (!nodes_subset(*new, node_states[N_HIGH_MEMORY])) {
 		err = -EINVAL;
-		goto out;
+		goto unlock_out;
 	}
 
 	err = security_task_movememory(task);
 	if (err)
-		goto out;
+		goto unlock_out;
 
+	rcu_read_unlock();
 	err = do_migrate_pages(mm, old, new,
 		capable(CAP_SYS_NICE) ? MPOL_MF_MOVE_ALL : MPOL_MF_MOVE);
 out:

@@ -1370,6 +1366,10 @@ out:
 	NODEMASK_SCRATCH_FREE(scratch);
 
 	return err;
+
+unlock_out:
+	rcu_read_unlock();
+	goto out;
 }
 
 

Index: linux-2.6/mm/migrate.c
===================================================================

mm/migrate.c +11 -13

@@ -1176,20 +1176,17 @@ set_status:
  * Migrate an array of page address onto an array of nodes and fill
  * the corresponding array of status.
  */
-static int do_pages_move(struct mm_struct *mm, struct task_struct *task,
+static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 			 unsigned long nr_pages,
 			 const void __user * __user *pages,
 			 const int __user *nodes,
 			 int __user *status, int flags)
 {
 	struct page_to_node *pm;
-	nodemask_t task_nodes;
 	unsigned long chunk_nr_pages;
 	unsigned long chunk_start;
 	int err;
 
-	task_nodes = cpuset_mems_allowed(task);
-
 	err = -ENOMEM;
 	pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
 	if (!pm)

@@ -1351,6 +1348,7 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 	struct task_struct *task;
 	struct mm_struct *mm;
 	int err;
+	nodemask_t task_nodes;
 
 	/* Check flags */
 	if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))

@@ -1367,10 +1365,11 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 		return -ESRCH;
 	}
 	mm = get_task_mm(task);
-	rcu_read_unlock();
 
-	if (!mm)
+	if (!mm) {
+		rcu_read_unlock();
 		return -EINVAL;
+	}
 
 	/*
 	 * Check if this process has the right to modify the specified

@@ -1378,24 +1377,23 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 	 * capabilities, superuser privileges or the same
 	 * userid as the target process.
 	 */
-	rcu_read_lock();
 	tcred = __task_cred(task);
 	if (cred->euid != tcred->suid && cred->euid != tcred->uid &&
 	    cred->uid  != tcred->suid && cred->uid  != tcred->uid &&
 	    !capable(CAP_SYS_NICE)) {
-		rcu_read_unlock();
 		err = -EPERM;
-		goto out;
+		goto unlock_out;
 	}
-	rcu_read_unlock();
 
  	err = security_task_movememory(task);
  	if (err)
-		goto out;
+		goto unlock_out;
 
+	task_nodes = cpuset_mems_allowed(task);
+	rcu_read_unlock();
 	if (nodes) {
-		err = do_pages_move(mm, task, nr_pages, pages, nodes, status,
-				    flags);
+		err = do_pages_move(mm, task_nodes, nr_pages, pages, nodes,
+				status, flags);
 	} else {
 		err = do_pages_stat(mm, nr_pages, pages, status);
 	}

Subject: migration: Do not do rcu_read_unlock until the last time we need the task_struct pointer

Migration functions perform the rcu_read_unlock too early. As a result the task pointed to may change. Bugs were introduced when adding security checks because rcu_unlock/lock sequences were inserted. Plus the security checks and do_move_pages used the task_struct pointer after rcu_unlock.

Fix those issues by removing the unlock/lock sequences and moving the rcu_read_unlock after the last use of the task struct pointer.

Signed-off-by Christoph Lameter <[email protected]>

Quoted 7 lines · Christoph Lameter

Subject: migration: Do not do rcu_read_unlock until the last time we need the task_struct pointer

Migration functions perform the rcu_read_unlock too early. As a result the task pointed to may change. Bugs were introduced when adding security checks because rcu_unlock/lock sequences were inserted. Plus the security checks and do_move_pages used the task_struct pointer after rcu_unlock.

Quoted 4 lines

Fix those issues by removing the unlock/lock sequences and moving the rcu_read_unlock after the last use of the task struct pointer.

Signed-off-by Christoph Lameter <[email protected]>

It doesn't fix the code duplication, but it definitely does fix the bug

Could you do another patch that removed the duplication?

cpuset_mems_allowed takes a mutex. Hmmm... Under rcu?

security_task_movememory does some kind of security hook.

Is that all safe? If not then we need to take a refcount on the task struct after all.

Quoted 2 lines

Is that all safe? If not then we need to take a refcount on the task struct after all.

Urg, no we can't sleep under an rcu_read_lock().

(quoted diff) +0 -1

@@ -1318,10 +1318,10 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	rcu_read_lock();
 	task = pid ? find_task_by_vpid(pid) : current;
 	if (!task) {
-		rcu_read_unlock();
 		err = -ESRCH;
 		goto out;
 	}

Quoted 4 lines · Dave Hansen

Quoted 2 lines

Is that all safe? If not then we need to take a refcount on the task struct after all.

Urg, no we can't sleep under an rcu_read_lock().

Ok so take a count and drop it before entering the main migration function?

---
 mm/mempolicy.c |   12 +++++++-----
 mm/migrate.c   |   20 +++++++++++---------
 2 files changed, 18 insertions(+), 14 deletions(-)

Index: linux-2.6/mm/mempolicy.c
===================================================================

mm/mempolicy.c +1 -1

@@ -1293,7 +1293,7 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 {
 
 	const struct cred *cred = current_cred(), *tcred;
 	struct mm_struct *mm = NULL;
-	struct task_struct *task;
+	struct task_struct *task = NULL;
 	nodemask_t task_nodes;
 	int err;

mm/mempolicy.c +6 -4

 	nodemask_t *old;

@@ -1318,10 +1318,10 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	rcu_read_lock();
 	task = pid ? find_task_by_vpid(pid) : current;
 	if (!task) {
-		rcu_read_unlock();
 		err = -ESRCH;
 		goto out;
 	}
+	get_task_struct(task);
 	mm = get_task_mm(task);
 	rcu_read_unlock();
 

@@ -1335,16 +1335,13 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	 * capabilities, superuser privileges or the same
 	 * userid as the target process.
 	 */
-	rcu_read_lock();
 	tcred = __task_cred(task);
 	if (cred->euid != tcred->suid && cred->euid != tcred->uid &&
 	    cred->uid  != tcred->suid && cred->uid  != tcred->uid &&
 	    !capable(CAP_SYS_NICE)) {
-		rcu_read_unlock();
 		err = -EPERM;
 		goto out;
 	}
-	rcu_read_unlock();
 
 	task_nodes = cpuset_mems_allowed(task);
 	/* Is the user allowed to access the target nodes? */

@@ -1362,9 +1359,14 @@ SYSCALL_DEFINE4(migrate_pages, pid_t, pi
 	if (err)
 		goto out;
 
+	put_task_struct(task);
+	task = NULL;
 	err = do_migrate_pages(mm, old, new,
 		capable(CAP_SYS_NICE) ? MPOL_MF_MOVE_ALL : MPOL_MF_MOVE);
 out:
+	if (task)
+		put_task_struct(task);
+
 	if (mm)
 		mmput(mm);
 	NODEMASK_SCRATCH_FREE(scratch);

Index: linux-2.6/mm/migrate.c
===================================================================

mm/migrate.c +9 -9

@@ -1176,20 +1176,17 @@ set_status:
  * Migrate an array of page address onto an array of nodes and fill
  * the corresponding array of status.
  */
-static int do_pages_move(struct mm_struct *mm, struct task_struct *task,
+static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 			 unsigned long nr_pages,
 			 const void __user * __user *pages,
 			 const int __user *nodes,
 			 int __user *status, int flags)
 {
 	struct page_to_node *pm;
-	nodemask_t task_nodes;
 	unsigned long chunk_nr_pages;
 	unsigned long chunk_start;
 	int err;
 
-	task_nodes = cpuset_mems_allowed(task);
-
 	err = -ENOMEM;
 	pm = (struct page_to_node *)__get_free_page(GFP_KERNEL);
 	if (!pm)

@@ -1351,6 +1348,7 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 	struct task_struct *task;
 	struct mm_struct *mm;
 	int err;
+	nodemask_t task_nodes;
 
 	/* Check flags */
 	if (flags & ~(MPOL_MF_MOVE|MPOL_MF_MOVE_ALL))

@@ -1366,6 +1364,7 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 		rcu_read_unlock();
 		return -ESRCH;
 	}
+	get_task_struct(task);
 	mm = get_task_mm(task);
 	rcu_read_unlock();
 

@@ -1378,30 +1377,33 @@ SYSCALL_DEFINE6(move_pages, pid_t, pid,
 	 * capabilities, superuser privileges or the same
 	 * userid as the target process.
 	 */
-	rcu_read_lock();
 	tcred = __task_cred(task);
 	if (cred->euid != tcred->suid && cred->euid != tcred->uid &&
 	    cred->uid  != tcred->suid && cred->uid  != tcred->uid &&
 	    !capable(CAP_SYS_NICE)) {
-		rcu_read_unlock();
 		err = -EPERM;
 		goto out;
 	}
-	rcu_read_unlock();
 
  	err = security_task_movememory(task);
  	if (err)
 		goto out;
 
+	task_nodes = cpuset_mems_allowed(task);
+	put_task_struct(task);
+	task = NULL;
+
 	if (nodes) {
-		err = do_pages_move(mm, task, nr_pages, pages, nodes, status,
-				    flags);
+		err = do_pages_move(mm, task_nodes, nr_pages, pages, nodes,
+				status, flags);

Taking a quick look it does appear that in cpuset_mems_allowed and it's cousins we never sleep under "callback_mutex" so that lock looks like it could become a spinlock.

But I have to say something just bothers me about the permissions for modifying an mm living in the task. We can have different rules for modifying an mm depending on the path to tme mm?

Especially in things like which numa nodes we can put pages in?

So by specifying a different pid to access them mm through the call can either work or succeed? Are these checks really sane?

Quoted 4 lines · Dave Hansen

Quoted 2 lines

Is that all safe? If not then we need to take a refcount on the task struct after all.

Urg, no we can't sleep under an rcu_read_lock().

Ok so take a count and drop it before entering the main migration function?

Quoted 2 lines

Ok so take a count and drop it before entering the main migration function?

For correct operation of kernel code a count sounds fine.

If you are going to allow sleeping how do you ensure that an exec that happens between the taking of the reference count and checking the permissions does not mess things up.

At the moment I suspect the permissions checks are not safe unless performed under both rcu_read_lock and task_lock to ensure that the task<->mm association does not change on us while we are working. Even with that the cred can change under us but at least we know the cred will be valid until rcu_read_unlock happens.

This entire thinhg of modifying another process is a pain.

Perhaps you can play with task->self_exec_id to detect an exec and fail the system call if there was an exec in between when we find the task and when we drop the task reference.

Quoted 8 lines · Eric W. Biederman

Quoted 2 lines

Ok so take a count and drop it before entering the main migration function?

For correct operation of kernel code a count sounds fine.

If you are going to allow sleeping how do you ensure that an exec that happens between the taking of the reference count and checking the permissions does not mess things up.

Ok in that case there is a race between which of the two address space structures (mm structs) are used. But that is up to the user to resolve if he wants to.

Quoted 5 lines

At the moment I suspect the permissions checks are not safe unless performed under both rcu_read_lock and task_lock to ensure that the task<->mm association does not change on us while we are working. Even with that the cred can change under us but at least we know the cred will be valid until rcu_read_unlock happens.

The permissions check only refer to the task struct.

Quoted 5 lines

This entire thinhg of modifying another process is a pain.

Perhaps you can play with task->self_exec_id to detect an exec and fail the system call if there was an exec in between when we find the task and when we drop the task reference.

I am not sure why there would be an issue. We have to operate on one mm the pid refers to. If it changes then we may either operate on the old one or the new one.

We can move the determination of the mm to the last point possible to show that it is not used earlier?

The problem that I see is that we may race with a suid exec in which case the permissions checks might pass for the pre-exec state and then we get the post exec mm that we don't actually have permissions for, but we manipulate it anyway.

So we really need to do something silly like get task and task->self_exec_id. Then perform the permission checks and get the mm. Then if just before we perform the operation task->self_exec_id is different restart the system call, or fail with something like -EAGAIN.

Migration functions perform the rcu_read_unlock too early. As a result the task pointed to may change from under us.

The following patch extend the period of the rcu_read_lock until after the permissions checks are done. We also take a refcount so that the task reference is stable when calling security check functions and performing cpuset node validation (which takes a mutex).

The refcount is dropped before actual page migration occurs so there is no change to the refcounts held during page migration.

Also move the determination of the mm of the task struct to immediately before the do_migrate*() calls so that it is clear that we switch from handling the task during permission checks to the mm for the actual migration. Since the determination is only done once and we then no longer use the task_struct we can be sure that we operate on a specific address space that will not change from under us.

What was the user-visible impact of the bug?

What was the user-visible impact of the bug?

Quoted 4 lines · Eric W. Biederman

Quoted 3 lines

The bug in migrate_pages() is that we do a rcu_unlock and a rcu_lock. If we drop those then we should be safe if the use of a task pointer within a rcu section is safe without taking a refcount.

Quoted 8 lines

Yes the user of a task_struct pointer found via a userspace pid is valid for the life of an rcu critical section, and the bug is indeed that we drop the rcu_lock and somehow expect the task to remain valid.

The guarantee comes from release_task. In release_task we call __exit_signal which calls __unhash_process, and then we call delayed_put_task to guarantee that the task lives until the end of the rcu interval.

Ah. Ok. Great.

Quoted 2 lines

In migrate_pages we have a lot of task accesses outside of the rcu critical section, and without a reference count on task.

Yes but that is only of interesting for setup and verification of permissions. What matters during migration is that the mm_struct does not go away and we take a refcount on that one.

Quoted 2 lines

I tell you the truth trying to figure out what that code needs to be correct if task != current makes my head hurt.

Hmm...

Quoted 6 lines

I think we need to grab a reference on task_struct, to stop the task from going away, and in addition we need to hold task_lock. To keep task->mm from changing (see exec_mmap). But we can't do that and sleep so I think the entire function needs to be rewritten, and the need for task deep in the migrate_pages path needs to be removed as even with the reference count held we can race with someone calling exec.

We dont need the task during migration. We only need the mm. The task is safe until rcu_read_unlock therefore maybe the following should fix migrate pages:

Subject: migration: Do not do rcu_read_unlock until the last time we need the task_struct pointer

Migration functions perform the rcu_read_unlock too early. As a result the task pointed to may change. Bugs were introduced when adding security checks because rcu_unlock/lock sequences were inserted. Plus the security checks and do_move_pages used the task_struct pointer after rcu_unlock.

Fix those issues by removing the unlock/lock sequences and moving the rcu_read_unlock after the last use of the task struct pointer.

Signed-off-by Christoph Lameter<[email protected]>