The biggest crawly horror I've found so far in auditing the tty locking
is current->signal->tty. The tty layer currently and explicitly protects
this using tty_mutex. The core kernel likewise knows about this.
Unfortunately:
SELinux doesn't do any locking at all
Dquot passes the tty to tty_write_message without locking
audit_log_exit doesn't do any locking at all
acct.c thinks tasklist_lock protects it (wrong)
drivers/char/sx misuses it unlocked in debug info
fs/proc/array thinks tasklist_lock will save it (also wrong)
fs3270 does fascinating things with it which don't look safe
ebtables remote debugging (#if 0 thankfully) does no locking
and just for fun calls the tty driver directly with no
driver locking either.
voyager_thread sets up a thread and then touches ->tty unlocked
(and it seems daemonize already fixed it)
Sparc solaris_procids sets it to NULL without locking
arch/ia64/kernel/unanligned seems to write to it without locking
arch/um/kernel/exec.c appears to believe task_lock is used
The semantics are actually as follows
signal->tty must not be changed without holding tty_mutex
signal->tty must not be used unless tty_mutex is held from before
reading it to completing using it
Simple if(signal->tty == NULL) type checks are ok
I'm looking longer term at tty ref counting and the like but for now and
current distributions it might be an idea to fix the existing problems.
Alan
Alan Cox wrote:
> The biggest crawly horror I've found so far in auditing the tty locking
> is current->signal->tty. The tty layer currently and explicitly protects
> this using tty_mutex. The core kernel likewise knows about this.
>
> Unfortunately:
> Dquot passes the tty to tty_write_message without locking
> arch/ia64/kernel/unanligned seems to write to it without locking
these two have absolutely no business at all using anything tty.... they should
just use printk with KERN_EMERG or whatever
Ar Maw, 2006-08-08 am 08:10 -0700, ysgrifennodd Arjan van de Ven:
> > Unfortunately:
> > Dquot passes the tty to tty_write_message without locking
> > arch/ia64/kernel/unanligned seems to write to it without locking
>
> these two have absolutely no business at all using anything tty.... they should
> just use printk with KERN_EMERG or whatever
Dquot does - it writes to the controlling tty of the process exceeding
quota . That is standard Unix behaviour
On Tue, Aug 08, 2006 at 04:44:36PM +0100, Alan Cox wrote:
> Ar Maw, 2006-08-08 am 08:10 -0700, ysgrifennodd Arjan van de Ven:
> > > Unfortunately:
> > > Dquot passes the tty to tty_write_message without locking
> > > arch/ia64/kernel/unanligned seems to write to it without locking
> >
> > these two have absolutely no business at all using anything tty.... they should
> > just use printk with KERN_EMERG or whatever
>
> Dquot does - it writes to the controlling tty of the process exceeding
> quota . That is standard Unix behaviour
IA-64 is also trying to be helpful by putting the message where
the user may actually see it (following the dquot precedent). But
the whole subject of whether we should print any messages for
unaligned accesses at all is rather controversial. So its a 50-50
shot whether I'll fix it by adding the mutex_lock/mutex_unlock
around the use of current->signal->tty, or just rip this out and
just leave the printk().
-Tony
On Tue, 2006-08-08 at 16:17 +0100, Alan Cox wrote:
> The biggest crawly horror I've found so far in auditing the tty locking
> is current->signal->tty. The tty layer currently and explicitly protects
> this using tty_mutex. The core kernel likewise knows about this.
>
> Unfortunately:
> SELinux doesn't do any locking at all
> Dquot passes the tty to tty_write_message without locking
> audit_log_exit doesn't do any locking at all
> acct.c thinks tasklist_lock protects it (wrong)
> drivers/char/sx misuses it unlocked in debug info
> fs/proc/array thinks tasklist_lock will save it (also wrong)
> fs3270 does fascinating things with it which don't look safe
> ebtables remote debugging (#if 0 thankfully) does no locking
> and just for fun calls the tty driver directly with no
> driver locking either.
> voyager_thread sets up a thread and then touches ->tty unlocked
> (and it seems daemonize already fixed it)
> Sparc solaris_procids sets it to NULL without locking
> arch/ia64/kernel/unanligned seems to write to it without locking
> arch/um/kernel/exec.c appears to believe task_lock is used
>
> The semantics are actually as follows
>
> signal->tty must not be changed without holding tty_mutex
> signal->tty must not be used unless tty_mutex is held from before
> reading it to completing using it
> Simple if(signal->tty == NULL) type checks are ok
>
> I'm looking longer term at tty ref counting and the like but for now and
> current distributions it might be an idea to fix the existing problems.
Does this look sane? Or do we need a common helper factored from
disassociate_ctty()? Why is the locking different for TIOCNOTTY in the
non-leader case?
---
selinux: fix tty locking
Take tty_mutex when accessing ->signal->tty.
Noted by Alan Cox.
Signed-off-by: Stephen Smalley <[email protected]>
---
security/selinux/hooks.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5d1b8c7..4b0f904 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1711,10 +1711,12 @@ static inline void flush_unauthorized_fi
{
struct avc_audit_data ad;
struct file *file, *devnull = NULL;
- struct tty_struct *tty = current->signal->tty;
+ struct tty_struct *tty;
struct fdtable *fdt;
long j = -1;
+ mutex_lock(&tty_mutex);
+ tty = current->signal->tty;
if (tty) {
file_list_lock();
file = list_entry(tty->tty_files.next, typeof(*file), f_u.fu_list);
@@ -1734,6 +1736,7 @@ static inline void flush_unauthorized_fi
}
file_list_unlock();
}
+ mutex_unlock(&tty_mutex);
/* Revalidate access to inherited open files. */
--
Stephen Smalley
National Security Agency
Ar Maw, 2006-08-08 am 13:11 -0400, ysgrifennodd Stephen Smalley:
> Does this look sane? Or do we need a common helper factored from
> disassociate_ctty()? Why is the locking different for TIOCNOTTY in the
> non-leader case?
The non-leader case for TIOCNOTTY in the base kernel is different
because it is wrong and I've fixed that one.
If you can factor disassociate_ctty out to do what you need I'd prefer
that path so the tty locking actually ends up in the tty layer.
> + mutex_lock(&tty_mutex);
> + tty = current->signal->tty;
> if (tty) {
> file_list_lock();
Looks sane and the lock ordering matches vhangup() which may actually
also do what you want - I'm not 100% sure I follow what SELinux tries to
do here.
Ar Maw, 2006-08-08 am 09:41 -0700, ysgrifennodd Luck, Tony:
> unaligned accesses at all is rather controversial. So its a 50-50
> shot whether I'll fix it by adding the mutex_lock/mutex_unlock
> around the use of current->signal->tty, or just rip this out and
> just leave the printk().
Personally I'd just rip it out full stop. Its trivial to use kprobes and
friends to audit such things if there is a performance concern.
Alan
On Tue, 2006-08-08 at 18:43 +0100, Alan Cox wrote:
> Ar Maw, 2006-08-08 am 13:11 -0400, ysgrifennodd Stephen Smalley:
> > Does this look sane? Or do we need a common helper factored from
> > disassociate_ctty()? Why is the locking different for TIOCNOTTY in the
> > non-leader case?
>
> The non-leader case for TIOCNOTTY in the base kernel is different
> because it is wrong and I've fixed that one.
>
> If you can factor disassociate_ctty out to do what you need I'd prefer
> that path so the tty locking actually ends up in the tty layer.
>
> > + mutex_lock(&tty_mutex);
> > + tty = current->signal->tty;
> > if (tty) {
> > file_list_lock();
>
> Looks sane and the lock ordering matches vhangup() which may actually
> also do what you want - I'm not 100% sure I follow what SELinux tries to
> do here.
SELinux is just revalidating access to the tty when the task changes
contexts upon execve, and resetting the tty if the task is no longer
allowed to use it. Likewise with the open file descriptors that would
be inherited. No clearing of the ttys of other tasks required as far as
SELinux is concerned, although that might not fit with normal semantics.
--
Stephen Smalley
National Security Agency
Ar Maw, 2006-08-08 am 13:44 -0400, ysgrifennodd Stephen Smalley:
> SELinux is just revalidating access to the tty when the task changes
> contexts upon execve, and resetting the tty if the task is no longer
> allowed to use it. Likewise with the open file descriptors that would
> be inherited. No clearing of the ttys of other tasks required as far as
> SELinux is concerned, although that might not fit with normal semantics.
The kernel requires you end up with a session leader etc so an exec that
loses rights by the session leader does indeed match disassociate_ctty I
guess. The ctty is a bit of an odd thing in the Unix world and perhaps
something that shouldn't have happened in that it gives you ability to
do things even if you have no fd to it.
Alan
On Tue, 2006-08-08 at 18:43 +0100, Alan Cox wrote:
> Ar Maw, 2006-08-08 am 13:11 -0400, ysgrifennodd Stephen Smalley:
> > Does this look sane? Or do we need a common helper factored from
> > disassociate_ctty()? Why is the locking different for TIOCNOTTY in the
> > non-leader case?
>
> The non-leader case for TIOCNOTTY in the base kernel is different
> because it is wrong and I've fixed that one.
>
> If you can factor disassociate_ctty out to do what you need I'd prefer
> that path so the tty locking actually ends up in the tty layer.
Possible patch below, but not yet tested. This splits the handling
between the tty layer and SELinux. Not sure about the !leader case,
should be consistent with whatever you end up using for TIOCNOTTY. This
changes behavior for SELinux in the leader case, since it will now
perform a full disassociate. Not sure whether the use of ->tty_files
should also be moved into the tty layer, with the tty layer then just
passing the file struct to the security hook instead of the tty struct.
---
drivers/char/tty_io.c | 30 ++++++++++++++++++++++++++-
include/linux/security.h | 20 ++++++++++++++++++
include/linux/tty.h | 1
security/dummy.c | 6 +++++
security/selinux/hooks.c | 51 ++++++++++++++++++++++++++++-------------------
5 files changed, 87 insertions(+), 21 deletions(-)
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index bfdb902..c16e0b3 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -94,6 +94,7 @@ #include <linux/idr.h>
#include <linux/wait.h>
#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/security.h>
#include <asm/uaccess.h>
#include <asm/system.h>
@@ -1175,8 +1176,11 @@ EXPORT_SYMBOL(tty_hung_up_p);
*
* The argument on_exit is set to 1 if called when a process is
* exiting; it is 0 if called by the ioctl TIOCNOTTY.
+ * The argument cond_sec is set to 1 if called by a security module
+ * to revalidate access to the ctty and conditionally disassociate
+ * if access is no longer permitted.
*/
-void disassociate_ctty(int on_exit)
+static void disassociate_ctty_core(int on_exit, int cond_sec)
{
struct tty_struct *tty;
struct task_struct *p;
@@ -1186,6 +1190,13 @@ void disassociate_ctty(int on_exit)
mutex_lock(&tty_mutex);
tty = current->signal->tty;
+ if (tty && cond_sec) {
+ if (!security_tty_disassociate(tty)) {
+ mutex_unlock(&tty_mutex);
+ unlock_kernel();
+ return;
+ }
+ }
if (tty) {
tty_pgrp = tty->pgrp;
mutex_unlock(&tty_mutex);
@@ -1222,6 +1233,23 @@ void disassociate_ctty(int on_exit)
unlock_kernel();
}
+void disassociate_ctty(int on_exit)
+{
+ disassociate_ctty_core(on_exit, 0);
+}
+
+void revalidate_ctty(void)
+{
+ if (current->signal->leader) {
+ disassociate_ctty_core(0, 1);
+ return;
+ }
+ mutex_lock(&tty_mutex);
+ if (security_tty_disassociate(current->signal->tty))
+ current->signal->tty = NULL;
+ mutex_unlock(&tty_mutex);
+}
+
void stop_tty(struct tty_struct *tty)
{
if (tty->stopped)
diff --git a/include/linux/security.h b/include/linux/security.h
index 6bc2aad..17ec32d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -500,6 +500,14 @@ #ifdef CONFIG_SECURITY
* @file contains the file structure being received.
* Return 0 if permission is granted.
*
+ * Security hooks for tty operations.
+ *
+ * @tty_disassociate:
+ * This hook allows security modules to control whether the tty
+ * should be disassociated.
+ * @tty contains the tty structure.
+ * Return 0 if permission is granted to the tty, or <0 to disassociate.
+ *
* Security hooks for task operations.
*
* @task_create:
@@ -1227,6 +1235,8 @@ struct security_operations {
struct fown_struct * fown, int sig);
int (*file_receive) (struct file * file);
+ int (*tty_disassociate) (struct tty_struct *tty);
+
int (*task_create) (unsigned long clone_flags);
int (*task_alloc_security) (struct task_struct * p);
void (*task_free_security) (struct task_struct * p);
@@ -1813,6 +1823,11 @@ static inline int security_file_receive
return security_ops->file_receive (file);
}
+static inline int security_tty_disassociate (struct tty_struct *tty)
+{
+ return security_ops->tty_disassociate (tty);
+}
+
static inline int security_task_create (unsigned long clone_flags)
{
return security_ops->task_create (clone_flags);
@@ -2488,6 +2503,11 @@ static inline int security_file_receive
return 0;
}
+static inline int security_tty_disassociate (struct tty_struct *tty)
+{
+ return 0;
+}
+
static inline int security_task_create (unsigned long clone_flags)
{
return 0;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e421d5e..c0f1fcb 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -292,6 +292,7 @@ extern void tty_vhangup(struct tty_struc
extern void tty_unhangup(struct file *filp);
extern int tty_hung_up_p(struct file * filp);
extern void do_SAK(struct tty_struct *tty);
+extern void revalidate_ctty(void);
extern void disassociate_ctty(int priv);
extern void tty_flip_buffer_push(struct tty_struct *tty);
extern int tty_get_baud_rate(struct tty_struct *tty);
diff --git a/security/dummy.c b/security/dummy.c
index 58c6d39..ca833b4 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -459,6 +459,11 @@ static int dummy_file_receive (struct fi
return 0;
}
+static int dummy_tty_disassociate (struct tty_struct *tty)
+{
+ return 0;
+}
+
static int dummy_task_create (unsigned long clone_flags)
{
return 0;
@@ -987,6 +992,7 @@ void security_fixup_ops (struct security
set_to_dummy_if_null(ops, file_set_fowner);
set_to_dummy_if_null(ops, file_send_sigiotask);
set_to_dummy_if_null(ops, file_receive);
+ set_to_dummy_if_null(ops, tty_disassociate);
set_to_dummy_if_null(ops, task_create);
set_to_dummy_if_null(ops, task_alloc_security);
set_to_dummy_if_null(ops, task_free_security);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5d1b8c7..278eb37 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1711,29 +1711,17 @@ static inline void flush_unauthorized_fi
{
struct avc_audit_data ad;
struct file *file, *devnull = NULL;
- struct tty_struct *tty = current->signal->tty;
struct fdtable *fdt;
long j = -1;
- if (tty) {
- file_list_lock();
- file = list_entry(tty->tty_files.next, typeof(*file), f_u.fu_list);
- if (file) {
- /* Revalidate access to controlling tty.
- Use inode_has_perm on the tty inode directly rather
- than using file_has_perm, as this particular open
- file may belong to another process and we are only
- interested in the inode-based check here. */
- struct inode *inode = file->f_dentry->d_inode;
- if (inode_has_perm(current, inode,
- FILE__READ | FILE__WRITE, NULL)) {
- /* Reset controlling tty. */
- current->signal->tty = NULL;
- current->signal->tty_old_pgrp = 0;
- }
- }
- file_list_unlock();
- }
+ /*
+ * Revalidate access to the controlling tty, and
+ * reset it if necessary.
+ * Calls back to selinux_tty_disassociate to check
+ * permissions after locking.
+ */
+ if (current->signal->tty)
+ revalidate_ctty();
/* Revalidate access to inherited open files. */
@@ -2650,6 +2638,27 @@ static int selinux_file_receive(struct f
return file_has_perm(current, file, file_to_av(file));
}
+static int selinux_tty_disassociate(struct tty_struct *tty)
+{
+ struct file *file;
+ int rc = 0;
+
+ file_list_lock();
+ file = list_entry(tty->tty_files.next, typeof(*file), f_u.fu_list);
+ if (file) {
+ /* Revalidate access to controlling tty.
+ Use inode_has_perm on the tty inode directly rather
+ than using file_has_perm, as this particular open
+ file may belong to another process and we are only
+ interested in the inode-based check here. */
+ struct inode *inode = file->f_dentry->d_inode;
+ rc = inode_has_perm(current, inode, FILE__READ | FILE__WRITE,
+ NULL);
+ }
+ file_list_unlock();
+ return rc;
+}
+
/* task security operations */
static int selinux_task_create(unsigned long clone_flags)
@@ -4538,6 +4547,8 @@ static struct security_operations selinu
.file_send_sigiotask = selinux_file_send_sigiotask,
.file_receive = selinux_file_receive,
+ .tty_disassociate = selinux_tty_disassociate,
+
.task_create = selinux_task_create,
.task_alloc_security = selinux_task_alloc_security,
.task_free_security = selinux_task_free_security,
--
Stephen Smalley
National Security Agency
> Ar Maw, 2006-08-08 am 08:10 -0700, ysgrifennodd Arjan van de Ven:
> > > Unfortunately:
> > > Dquot passes the tty to tty_write_message without locking
> > > arch/ia64/kernel/unanligned seems to write to it without locking
> >
> > these two have absolutely no business at all using anything tty.... they should
> > just use printk with KERN_EMERG or whatever
>
> Dquot does - it writes to the controlling tty of the process exceeding
> quota . That is standard Unix behaviour
Yes. On one hand I find this behaviour kind of inconsistent (Unix usually
just returns an error code and that's it) but on the other hand there is
no other good way of informing user about e.g. exceeded quota soft
limit.
So I'll just fix up the tty locking ;).
Honza
--
Jan Kara <[email protected]>
SuSE CR Labs
Stephen Smalley writes:
> SELinux is just revalidating access to the tty when the task
> changes contexts upon execve, and resetting the tty if the
> task is no longer allowed to use it. Likewise with the open
> file descriptors that would be inherited. No clearing of the
> ttys of other tasks required as far as SELinux is concerned,
> although that might not fit with normal semantics.
If the process goes back to the old context after a second
execve or via special rights, it ought regain access to the tty.
(just block access instead of resetting the tty)
>>>>> "Alan" == Alan Cox <[email protected]> writes:
Alan> Ar Maw, 2006-08-08 am 09:41 -0700, ysgrifennodd Luck, Tony:
>> unaligned accesses at all is rather controversial. So its a 50-50
>> shot whether I'll fix it by adding the mutex_lock/mutex_unlock
>> around the use of current->signal->tty, or just rip this out and
>> just leave the printk().
Alan> Personally I'd just rip it out full stop. Its trivial to use
Alan> kprobes and friends to audit such things if there is a
Alan> performance concern.
Personally I don't like the current approach. However, I believe the
philosophy behind it is that users rarely look in dmesg and they
should be notified (and beaten with a stick) when their badly written
app spawns unaligned accesses which end up being emulated by the
kernel.
These messages are normally caused by userland code, so kprobes
probably wont do much good :)
Cheers,
Jes
Ar Mer, 2006-08-09 am 04:09 -0400, ysgrifennodd Jes Sorensen:
> Personally I don't like the current approach. However, I believe the
> philosophy behind it is that users rarely look in dmesg and they
> should be notified (and beaten with a stick) when their badly written
> app spawns unaligned accesses which end up being emulated by the
> kernel.
The users won't seem the anyway, they are hidden behind the GUI.
> These messages are normally caused by userland code, so kprobes
> probably wont do much good :)
Jes, read up on kprobes a little if you think its of no use in these
kind of situations. A systemtap script to count/measure alignment fault
rates and see who is causing the load isn't very hard to write.
Alan
Alan Cox wrote:
> Ar Mer, 2006-08-09 am 04:09 -0400, ysgrifennodd Jes Sorensen:
>> Personally I don't like the current approach. However, I believe the
>> philosophy behind it is that users rarely look in dmesg and they
>> should be notified (and beaten with a stick) when their badly written
>> app spawns unaligned accesses which end up being emulated by the
>> kernel.
>
> The users won't seem the anyway, they are hidden behind the GUI.
Regularly see complaints from users in the HPC space over this .....
clearly it's the kernel thats broken, not their buggy code.
Jes
On Wed, Aug 09, 2006 at 11:44:10AM +0100, Alan Cox wrote:
>
> The users won't see them anyway, they are hidden behind the GUI.
I think that the majority of IA-64 users are connected to the target
machine via ssh login, rather than a directly connected VGA screen.
So they should see the message on their pty.
> > These messages are normally caused by userland code, so kprobes
> > probably wont do much good :)
>
> Jes, read up on kprobes a little if you think its of no use in these
> kind of situations. A systemtap script to count/measure alignment fault
> rates and see who is causing the load isn't very hard to write.
But this does make sense ... the system administrator of a mainframe or
super-computer can (and arguably should) be monitoring resource
utilization and providing feedback to users on how to get the best
performance from their applications.
-Tony (currently at 90% rip out this message, 10% add the mutex lock/unlock).
Luck, Tony wrote:
> On Wed, Aug 09, 2006 at 11:44:10AM +0100, Alan Cox wrote:
>> Jes, read up on kprobes a little if you think its of no use in these
>> kind of situations. A systemtap script to count/measure alignment fault
>> rates and see who is causing the load isn't very hard to write.
>
> But this does make sense ... the system administrator of a mainframe or
> super-computer can (and arguably should) be monitoring resource
> utilization and providing feedback to users on how to get the best
> performance from their applications.
Actually perfmon would probably be better suited for this on ia64 than
kprobes/systemtap. You can do it on hackish way with a [jk]probe, but it
isn't going to be all that pretty.
> -Tony (currently at 90% rip out this message, 10% add the mutex lock/unlock).
Nuke it!
Jes
> The biggest crawly horror I've found so far in auditing the tty locking
> is current->signal->tty. The tty layer currently and explicitly protects
> this using tty_mutex. The core kernel likewise knows about this.
>
> Unfortunately:
> SELinux doesn't do any locking at all
> Dquot passes the tty to tty_write_message without locking
Ok, is something like attached patch fine?
Honza