tty: fix leakage of -ERESTARTSYS to userland

Spotted by Satoru Takeuchi.

kill_pgrp(task_pgrp(current)) sends the signal to the current's thread group, but can choose any sub-thread as a target for signal_wake_up(). This means that job_control() and tty_check_change() may return -ERESTARTSYS without signal_pending().

Signed-off-by Oleg Nesterov <[email protected]>

drivers/char/n_tty.c renamed from drivers/char/n_tty.c~ +1 -0

@@ -1191,6 +1191,7 @@ static int job_control(struct tty_struct
 			    is_current_pgrp_orphaned())
 				return -EIO;
 			kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+			set_thread_flag(TIF_SIGPENDING);
 			return -ERESTARTSYS;
 		}
 	}

drivers/char/n_tty.c static int job_control(struct tty_struct · line 1197

Quoted 10 lines · On Tue, 29 May 2007 22:44:35 +0400 Oleg Nesterov <[email protected]> wrote:

drivers/char/n_tty.c renamed from drivers/char/n_tty.c~ +1 -0

@@ -1191,6 +1191,7 @@ static int job_control(struct tty_struct
 			    is_current_pgrp_orphaned())
 				return -EIO;
 			kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+			set_thread_flag(TIF_SIGPENDING);
 			return -ERESTARTSYS;
 		}
 	}

Are there other callers of kill_pgrp() which have the same problem?

Perhaps we should have a kill_pgrp_self() which takes care of doing this, rather than open-coding it. Something with a comment which explains what's going on ;)

Quoted 15 lines · Andrew Morton

drivers/char/n_tty.c static int job_control(struct tty_struct · line 1197

Quoted 10 lines · On Tue, 29 May 2007 22:44:35 +0400 Oleg Nesterov <[email protected]> wrote:

drivers/char/n_tty.c renamed from drivers/char/n_tty.c~ +1 -0

@@ -1191,6 +1191,7 @@ static int job_control(struct tty_struct
 			    is_current_pgrp_orphaned())
 				return -EIO;
 			kill_pgrp(task_pgrp(current), SIGTTIN, 1);
+			set_thread_flag(TIF_SIGPENDING);
 			return -ERESTARTSYS;
 		}
 	}

Are there other callers of kill_pgrp() which have the same problem?

Hopefully no.

Quoted 3 lines

Perhaps we should have a kill_pgrp_self() which takes care of doing this, rather than open-coding it. Something with a comment which explains what's going on ;)

This set_thread_flag(TIF_SIGPENDING) is "connected" to "return -ERESTARTSYS", not to kill_pgrp(), imho the new helper is not so suitable.

Perhaps it makes sense to add the comment into include/linux/errno.h, to explain that -ERESTART... codes are only valid when signal_pending() == true.

Aside from typos, I think it should be more clearly and strongly worded.

"These should never be seen by user programs. To return one of these codes, signal_pending() MUST be set. Note that ptrace can observe these at syscall exit tracing, but they will never be left for the debugged user process to see."