[patch] ptrace: unlocked access to last_siginfo (resending)

[patch] ptrace: unlocked access to last_siginfo (resending)

[pmeda]

Since Roland changed now to wakeup tracee with kill, I guess this needs to be fixed.
http://linus.bkbits.net:8080/linux-2.5/gnupatch@41e3fe5fIRH-W3aDnXZgfQ-qIvuXYg

ptrace_setsiginfo/ptrace_getsiginfo need to do locked access
to last_siginfo.  ptrace_notify()/ptrace_stop() sets the
current->last_siginfo and sleeps on schedule(). It can be waked
up by kill signal from signal_wake_up before debugger wakes it up.
On return from schedule(), the current->last_siginfo is reset.

Signed-off-by: Prasanna Meda <pmeda@akamai.com>


--- a/kernel/ptrace.c	Fri Nov 19 18:27:26 2004
+++ b/kernel/ptrace.c	Fri Nov 19 18:52:52 2004
@@ -303,18 +303,33 @@
 
 static int ptrace_getsiginfo(struct task_struct *child, siginfo_t __user * data)
 {
-	if (child->last_siginfo == NULL)
-		return -EINVAL;
-	return copy_siginfo_to_user(data, child->last_siginfo);
+	siginfo_t lastinfo;
+
+	spin_lock_irq(&child->sighand->siglock);
+	if (likely(child->last_siginfo != NULL)) {
+		memcpy(&lastinfo, child->last_siginfo, sizeof (siginfo_t));
+		spin_unlock_irq(&child->sighand->siglock);
+		return copy_siginfo_to_user(data, &lastinfo);
+	}
+	spin_unlock_irq(&child->sighand->siglock);
+	return -EINVAL;
 }
 
 static int ptrace_setsiginfo(struct task_struct *child, siginfo_t __user * data)
 {
-	if (child->last_siginfo == NULL)
-		return -EINVAL;
-	if (copy_from_user(child->last_siginfo, data, sizeof (siginfo_t)) != 0)
+	siginfo_t newinfo;
+
+	if (copy_from_user(&newinfo, data, sizeof (siginfo_t)) != 0)
 		return -EFAULT;
-	return 0;
+
+	spin_lock_irq(&child->sighand->siglock);
+	if (likely(child->last_siginfo != NULL)) {
+		memcpy(child->last_siginfo, &newinfo, sizeof (siginfo_t));
+		spin_unlock_irq(&child->sighand->siglock);
+		return 0;
+	}
+	spin_unlock_irq(&child->sighand->siglock);
+	return -EINVAL;
 }
 
 int ptrace_request(struct task_struct *child, long request,
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [patch] ptrace: unlocked access to last_siginfo (resending)

[Roland McGrath]

> Since Roland changed now to wakeup tracee with kill, I guess this needs to be fixed.
> http://linus.bkbits.net:8080/linux-2.5/gnupatch@41e3fe5fIRH-W3aDnXZgfQ-qIvuXYg
Indeed, this change should go in.  I'd forgotten about this.  I don't think
there are any other things we decided to leave one way or another based on
the ptrace behavior that has now changed back again, but I might be
forgetting others too.  Thanks for bringing it up.


Thanks,
Roland
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

Re: [patch] ptrace: unlocked access to last_siginfo (resending)

[Prasanna Meda]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

Roland McGrath wrote:

> > Since Roland changed now to wakeup tracee with kill, I guess this needs to be fixed.
> > http://linus.bkbits.net:8080/linux-2.5/gnupatch@41e3fe5fIRH-W3aDnXZgfQ-qIvuXYg
> Indeed, this change should go in.  I'd forgotten about this.  I don't think
> there are any other things we decided to leave one way or another based on
> the ptrace behavior that has now changed back again, but I might be
> forgetting others too.  Thanks for bringing it up.

  Thanks, but looks like we fixed only part of the problem. If the
  child is on the exit path and releases sighand, we need to check for
  its existence too.  The attached patch should work.


Thanks,
Prasanna.


[-- Attachment #2: ptrace_needs_tasklistlock.patch --]
[-- Type: text/plain, Size: 2329 bytes --]



Looks like we fixed only part of the problem earlier. When the child
moves away from ptrace notify and resets the last_siginfo, sighand lock
helps. But if the child goes further in exit and releases the sighand,
we need to test that case too.See ptrace_check_attach() and exit_sighand().
They also use the task_list_lock.


Signed-Off-by: Prasanna Meda <pmeda@akamai.com>


--- a/kernel/ptrace.c	Sun Jan 16 10:57:30 2005
+++ b/kernel/ptrace.c	Sun Jan 16 11:59:03 2005
@@ -320,32 +320,44 @@
 static int ptrace_getsiginfo(struct task_struct *child, siginfo_t __user * data)
 {
 	siginfo_t lastinfo;
+	int error = -ESRCH;
 
-	spin_lock_irq(&child->sighand->siglock);
-	if (likely(child->last_siginfo != NULL)) {
-		memcpy(&lastinfo, child->last_siginfo, sizeof (siginfo_t));
-		spin_unlock_irq(&child->sighand->siglock);
-		return copy_siginfo_to_user(data, &lastinfo);
+	read_lock_irq(&tasklist_lock);
+	if (likely(child->sighand != NULL)) {
+		error = -EINVAL;
+		spin_lock(&child->sighand->siglock);
+		if (likely(child->last_siginfo != NULL)) {
+			memcpy(&lastinfo, child->last_siginfo, sizeof (siginfo_t));
+			error = 0;
+		}
+		spin_unlock(&child->sighand->siglock);
 	}
-	spin_unlock_irq(&child->sighand->siglock);
-	return -EINVAL;
+	read_unlock_irq(&tasklist_lock);
+	if (!error)
+		return copy_siginfo_to_user(data, &lastinfo);
+	return error;
 }
 
 static int ptrace_setsiginfo(struct task_struct *child, siginfo_t __user * data)
 {
 	siginfo_t newinfo;
+	int error = -ESRCH;
 
-	if (copy_from_user(&newinfo, data, sizeof (siginfo_t)) != 0)
+	if (copy_from_user(&newinfo, data, sizeof (siginfo_t)))
 		return -EFAULT;
 
-	spin_lock_irq(&child->sighand->siglock);
-	if (likely(child->last_siginfo != NULL)) {
-		memcpy(child->last_siginfo, &newinfo, sizeof (siginfo_t));
-		spin_unlock_irq(&child->sighand->siglock);
-		return 0;
+	read_lock_irq(&tasklist_lock);
+	if (likely(child->sighand != NULL)) {
+		error = -EINVAL;
+		spin_lock(&child->sighand->siglock);
+		if (likely(child->last_siginfo != NULL)) {
+			memcpy(child->last_siginfo, &newinfo, sizeof (siginfo_t));
+			error = 0;
+		}
+		spin_unlock(&child->sighand->siglock);
 	}
-	spin_unlock_irq(&child->sighand->siglock);
-	return -EINVAL;
+	read_unlock_irq(&tasklist_lock);
+	return error;
 }
 
 int ptrace_request(struct task_struct *child, long request,

Re: [patch] ptrace: unlocked access to last_siginfo (resending)

[Roland McGrath]

>   Thanks, but looks like we fixed only part of the problem. If the
>   child is on the exit path and releases sighand, we need to check for
>   its existence too.  The attached patch should work.

That's correct.  Technically you don't need read_lock_irq, but just
spin_lock_irq, not that it really makes a difference.  Myself, I would
change that and also use struct assignment instead of memcpy.
But your patch is fine as it is.


Thanks,
Roland
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>

RE: [patch] ptrace: unlocked access to last_siginfo (resending)

[Meda, Prasanna]

>   That's correct.  Technically you don't need read_lock_irq, but just
>   spin_lock_irq, not that it really makes a difference.  Myself, I would
>   change that and also use struct assignment instead of memcpy.
>   But your patch is fine as it is.

Agreed, and I am going to preapre a patch based on your suggestions and
resend.

Thanks,
Prasanna
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"aart@kvack.org"> aart@kvack.org </a>