[PATCH] tty: fix leakage of -ERESTARTSYS to userland

[PATCH] tty: fix leakage of -ERESTARTSYS to userland

[Oleg Nesterov]

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 <oleg@tv-sign.ru>

--- t/drivers/char/n_tty.c~	2007-04-05 12:18:26.000000000 +0400
+++ t/drivers/char/n_tty.c	2007-05-28 10:57:58.000000000 +0400
@@ -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;
 		}
 	}
--- t/drivers/char/tty_io.c~	2007-04-05 12:18:26.000000000 +0400
+++ t/drivers/char/tty_io.c	2007-05-29 22:15:52.000000000 +0400
@@ -1121,7 +1121,8 @@ int tty_check_change(struct tty_struct *
 		return 0;
 	if (is_current_pgrp_orphaned())
 		return -EIO;
-	(void) kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+	kill_pgrp(task_pgrp(current), SIGTTOU, 1);
+	set_thread_flag(TIF_SIGPENDING);
 	return -ERESTARTSYS;
 }
 

Re: [PATCH] tty: fix leakage of -ERESTARTSYS to userland

[Andrew Morton]

On Tue, 29 May 2007 22:44:35 +0400
Oleg Nesterov <oleg@tv-sign.ru> wrote:

> 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 <oleg@tv-sign.ru>
> 
> --- t/drivers/char/n_tty.c~	2007-04-05 12:18:26.000000000 +0400
> +++ t/drivers/char/n_tty.c	2007-05-28 10:57:58.000000000 +0400
> @@ -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;
>  		}
>  	}
> --- t/drivers/char/tty_io.c~	2007-04-05 12:18:26.000000000 +0400
> +++ t/drivers/char/tty_io.c	2007-05-29 22:15:52.000000000 +0400
> @@ -1121,7 +1121,8 @@ int tty_check_change(struct tty_struct *
>  		return 0;
>  	if (is_current_pgrp_orphaned())
>  		return -EIO;
> -	(void) kill_pgrp(task_pgrp(current), SIGTTOU, 1);
> +	kill_pgrp(task_pgrp(current), SIGTTOU, 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 ;)

Re: [PATCH] tty: fix leakage of -ERESTARTSYS to userland

[Oleg Nesterov]

On 05/30, Andrew Morton wrote:
> On Tue, 29 May 2007 22:44:35 +0400
> Oleg Nesterov <oleg@tv-sign.ru> wrote:
> 
> > --- t/drivers/char/n_tty.c~	2007-04-05 12:18:26.000000000 +0400
> > +++ t/drivers/char/n_tty.c	2007-05-28 10:57:58.000000000 +0400
> > @@ -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.

> 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.

Oleg.

Re: [PATCH] tty: fix leakage of -ERESTARTSYS to userland

[Satoru Takeuchi]

At Wed, 30 May 2007 23:18:49 +0400,
Oleg Nesterov wrote:
> 
> On 05/30, Andrew Morton wrote:
> > On Tue, 29 May 2007 22:44:35 +0400
> > Oleg Nesterov <oleg@tv-sign.ru> wrote:
> > 
> > > --- t/drivers/char/n_tty.c~	2007-04-05 12:18:26.000000000 +0400
> > > +++ t/drivers/char/n_tty.c	2007-05-28 10:57:58.000000000 +0400
> > > @@ -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.
> 
> > 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.

Like this?

Satoru

---
Add comment for errnos related to restart syscall to avoid the leakage of
kernel only errnos.

Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Cc: Oleg Nesterov <oleg@tv-sign.ru>

Index: linux-2.6.22-rc3/include/linux/errno.h
===================================================================
--- linux-2.6.22-rc3.orig/include/linux/errno.h	2007-04-26 12:08:32.000000000 +0900
+++ linux-2.6.22-rc3/include/linux/errno.h	2007-05-31 09:44:27.000000000 +0900
@@ -5,7 +5,11 @@
 
 #ifdef __KERNEL__
 
-/* Should never be seen by user programs */
+/*
+ * Should never be seen by user programs. Please note that returing
+ * `ERESTART*' errnos when `!signal_pending()' incurs the leakage of these
+ * errnos to user space.
+ */
 #define ERESTARTSYS	512
 #define ERESTARTNOINTR	513
 #define ERESTARTNOHAND	514	/* restart if no handler.. */

Re: [PATCH] tty: fix leakage of -ERESTARTSYS to userland

[Roland McGrath]

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."


Thanks,
Roland

Re: [PATCH] tty: fix leakage of -ERESTARTSYS to userland

[Satoru Takeuchi]

At Wed, 30 May 2007 18:08:55 -0700 (PDT),
Roland McGrath wrote:
> 
> 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."

Thanks, is that better?

Satoru

---
Add comment for errnos related to restart syscall to avoid the leakage of
them to user programs.

Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Cc: Oleg Nesterov <oleg@tv-sign.ru>
Cc: Roland McGrath <roland@redhat.com>

Index: linux-2.6.22-rc3/include/linux/errno.h
===================================================================
--- linux-2.6.22-rc3.orig/include/linux/errno.h	2007-04-26 12:08:32.000000000 +0900
+++ linux-2.6.22-rc3/include/linux/errno.h	2007-05-31 10:47:40.000000000 +0900
@@ -5,7 +5,12 @@
 
 #ifdef __KERNEL__
 
-/* Should never be seen by user programs */
+/*
+ * These should never be seen by user programs.  To return one of ERESTART*
+ * 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.
+ */
 #define ERESTARTSYS	512
 #define ERESTARTNOINTR	513
 #define ERESTARTNOHAND	514	/* restart if no handler.. */

Re: [PATCH] tty: fix leakage of -ERESTARTSYS to userland

[Roland McGrath]

That looks good to me.


Thanks,
Roland