dcache_lock deadlock due to auditing

dcache_lock deadlock due to auditing

[Serge Hallyn]

Hi,

a team running some tcp benchmarks on new hardware found a recursive
spinlock(dcache_lock) deadlock.  It seems to always be caused by:

 A process is doing a d_alloc() which does a spin_lock(dcache_lock).
 It is interrupted to receive a packet
 selinux_sock_rcv_skb gets called
 it calls avc_has_perm
 this calls avc_audit
 this calls audit_log_d_path
 this calls d_path which does spin_lock(dcache_lock)

Deferring the actual audit work to the audit subsystem in a manner
similar to what Stephen's recent audit_log_exit() patch did seems one
possible way to solve this problem.  Another might be to determine our
context before the audit_log_d_path( vma->vm_file->f_dentry ), and, if
we can determine that we were called from interrupt, simply print out
the f_dentry->d_name.name itself.

Are there other solutions?

Currently the team is testing Stephen's patch (from march 31) to see
whether it is sufficient.

thanks,
-serge



-- 
Serge Hallyn <serue@us.ibm.com>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: dcache_lock deadlock due to auditing

[Stephen Smalley]

On Wed, 2005-04-13 at 12:36 -0500, Serge Hallyn wrote:
> a team running some tcp benchmarks on new hardware found a recursive
> spinlock(dcache_lock) deadlock.  It seems to always be caused by:
> 
>  A process is doing a d_alloc() which does a spin_lock(dcache_lock).
>  It is interrupted to receive a packet
>  selinux_sock_rcv_skb gets called
>  it calls avc_has_perm
>  this calls avc_audit
>  this calls audit_log_d_path
>  this calls d_path which does spin_lock(dcache_lock)
> 
> Deferring the actual audit work to the audit subsystem in a manner
> similar to what Stephen's recent audit_log_exit() patch did seems one
> possible way to solve this problem.  Another might be to determine our
> context before the audit_log_d_path( vma->vm_file->f_dentry ), and, if
> we can determine that we were called from interrupt, simply print out
> the f_dentry->d_name.name itself.

I think that in_softirq can have false positive, so I don't think we
want to try to test for it in avc_audit.  We could add a flag to the
avc_audit_data structure so that the caller of avc_has_perm* could
specify that avc_audit should not attempt to take the dcache lock, but
that seems ugly.

The cleanest solution in my mind is to remove all logging of pid, exe,
and comm from avc_audit as per my patch and handle that entirely in
audit_log_exit.  But that requires people to enable syscall auditing to
still get that information for avc denials, and will break compatibility
for seaudit, requiring it to be modified to acquire that information
from the subsequent syscall audit record.  One side benefit of this
solution is that it eliminates the confusion about bogus pid and exe
information on permission checks that are not performed in process
context like the socket receive checks and sigio delivery.

A less clean solution that preserves compatibility would be to still
include the pid=, exe=, and comm= information in avc_audit but only
include the last d_name component of the exe, as you suggest above.

One item to note is that the current avc_audit code includes support for
logging information about another task rather than current by having the
caller set the tsk field of the avc_audit_data structure prior to
calling avc_has_perm*.  That was originally included for replacing the
hardcoded capability checks by the OOM killer on possible target tasks
with calls to the LSM capable hook, but is not presently used as that
part of the LSM patch was never mainstreamed into 2.6.  I could also see
that potentially being useful for the task_kill hook, in order to get
information about the target task other than just its security context,
although in that case I would want information on both current and the
target task, which we presently don't allow for.

> Are there other solutions?
> 
> Currently the team is testing Stephen's patch (from march 31) to see
> whether it is sufficient.

-- 
Stephen Smalley <sds@tycho.nsa.gov>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: dcache_lock deadlock due to auditing

[Karl MacMillan]

> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> > Deferring the actual audit work to the audit subsystem in a manner
> > similar to what Stephen's recent audit_log_exit() patch did seems one
> > possible way to solve this problem.  Another might be to determine our
> > context before the audit_log_d_path( vma->vm_file->f_dentry ), and, if
> > we can determine that we were called from interrupt, simply print out
> > the f_dentry->d_name.name itself.
> 
> I think that in_softirq can have false positive, so I don't think we
> want to try to test for it in avc_audit.  We could add a flag to the
> avc_audit_data structure so that the caller of avc_has_perm* could
> specify that avc_audit should not attempt to take the dcache lock, but
> that seems ugly.
> 
> The cleanest solution in my mind is to remove all logging of pid, exe,
> and comm from avc_audit as per my patch and handle that entirely in
> audit_log_exit.  But that requires people to enable syscall auditing to
> still get that information for avc denials, and will break compatibility
> for seaudit, requiring it to be modified to acquire that information
> from the subsequent syscall audit record.  One side benefit of this
> solution is that it eliminates the confusion about bogus pid and exe
> information on permission checks that are not performed in process
> context like the socket receive checks and sigio delivery.
> 

We can certainly update seaudit to handle this and it certainly looks like this
is going to have to be done regardless this specific instance in the future. 

Just to be clear, this decision is really to continue the movement away from avc
messages being a complete source for audit message related to SELinux to both
avc and syscall auditing being required to get a full picture. My sense is that
this is the direction that things must go in for technical and political
reasons, though I have to say that my preference would be (for what it's worth)
for SELinux avc audit messages to be complete, self-contained, and be triggered
even with DAC permissions deny access.

At this point, my only real concern is that it be easier for users to get this
information and I simply don't know enough about the audit infrastructure to
know whether this will be the case or not. What kind of configuration will be
required to get the additional syscall audit messages?

Karl

---
Karl MacMillan
Tresys Technology
http://www.tresys.com
(410) 290-1411 ext 134

> A less clean solution that preserves compatibility would be to still
> include the pid=, exe=, and comm= information in avc_audit but only
> include the last d_name component of the exe, as you suggest above.
> 
> One item to note is that the current avc_audit code includes support for
> logging information about another task rather than current by having the
> caller set the tsk field of the avc_audit_data structure prior to
> calling avc_has_perm*.  That was originally included for replacing the
> hardcoded capability checks by the OOM killer on possible target tasks
> with calls to the LSM capable hook, but is not presently used as that
> part of the LSM patch was never mainstreamed into 2.6.  I could also see
> that potentially being useful for the task_kill hook, in order to get
> information about the target task other than just its security context,
> although in that case I would want information on both current and the
> target task, which we presently don't allow for.
> 
> > Are there other solutions?
> >
> > Currently the team is testing Stephen's patch (from march 31) to see
> > whether it is sufficient.
> 
> --
> Stephen Smalley <sds@tycho.nsa.gov>
> National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: dcache_lock deadlock due to auditing

[Stephen Smalley]

On Thu, 2005-04-14 at 12:06 -0400, Karl MacMillan wrote:
> Just to be clear, this decision is really to continue the movement away from avc
> messages being a complete source for audit message related to SELinux to both
> avc and syscall auditing being required to get a full picture. My sense is that
> this is the direction that things must go in for technical and political
> reasons, though I have to say that my preference would be (for what it's worth)
> for SELinux avc audit messages to be complete, self-contained, and be triggered
> even with DAC permissions deny access.

The SELinux avc audit messages will remain complete as far as having all
the information you need to tie it back to policy or generate policy
rules, i.e. the security contexts and class information.  The auxiliary
audit information (pid, exe, path, port, ipaddr, etc) has always just
been to help track down the causes of denials.  As far as being
triggered for DAC denials, that just isn't an option, as the LSM hooks
are placed after the DAC checks in most cases and won't be reached for
DAC denials.  That avoids filling the logs with MAC denials that would
have been denied due to DAC anyway (which is quite common due to
application and library probing of the environment) and doesn't leak
information (since both the DAC denial and the MAC denial yield the same
error code).  But if your real concern is capturing the relevant
security contexts upon a DAC denial, we can support that; SELinux
already provides hook functions for copying the contexts of processes
and files to buffers to support the /proc/pid/attr and xattr APIs, so
that audit framework can call those hooks and save that information for
processing by audit_log_exit upon DAC denials as well.

> At this point, my only real concern is that it be easier for users to get this
> information and I simply don't know enough about the audit infrastructure to
> know whether this will be the case or not. What kind of configuration will be
> required to get the additional syscall audit messages?

Boot your kernel with audit=1 or run auditctl -e 1 at any time.  That
enables syscall auditing.  Then, every time SELinux generates an audit
message via avc_audit, a syscall audit record will also be written at
syscall exit, and they can be tied together based on the
timestamp/serial number in the prefix.
 
-- 
Stephen Smalley <sds@tycho.nsa.gov>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: dcache_lock deadlock due to auditing

[Daniel H Jones]

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

"SELinux
already provides hook functions for copying the contexts of processes
and files to buffers to support the /proc/pid/attr and xattr APIs, so
that audit framework can call those hooks and save that information for
"processing by audit_log_exit upon DAC denials as well.

I'm guessing the security context obtained from the SELinux hooks are  is 
represented by a sid.
Is this true? If that's the case, can it be readily converted to the 
string representation for display
at audit_log_exit time? 

Thanks,
Dan Jones
IBM Linux Technology Center, Security
danjones@us.ibm.com



Stephen Smalley <sds@tycho.nsa.gov> 
Sent by: owner-selinux@tycho.nsa.gov
04/14/2005 11:15 AM

To
Karl MacMillan <kmacmillan@tresys.com>
cc
serue@us.ltcfwd.linux.ibm.com, selinux@tycho.nsa.gov, "'James Morris'" 
<jmorris@redhat.com>, selinux-dev@tresys.com, linux-audit@redhat.com
Subject
RE: dcache_lock deadlock due to auditing






On Thu, 2005-04-14 at 12:06 -0400, Karl MacMillan wrote:
> Just to be clear, this decision is really to continue the movement away 
from avc
> messages being a complete source for audit message related to SELinux to 
both
> avc and syscall auditing being required to get a full picture. My sense 
is that
> this is the direction that things must go in for technical and political
> reasons, though I have to say that my preference would be (for what it's 
worth)
> for SELinux avc audit messages to be complete, self-contained, and be 
triggered
> even with DAC permissions deny access.

The SELinux avc audit messages will remain complete as far as having all
the information you need to tie it back to policy or generate policy
rules, i.e. the security contexts and class information.  The auxiliary
audit information (pid, exe, path, port, ipaddr, etc) has always just
been to help track down the causes of denials.  As far as being
triggered for DAC denials, that just isn't an option, as the LSM hooks
are placed after the DAC checks in most cases and won't be reached for
DAC denials.  That avoids filling the logs with MAC denials that would
have been denied due to DAC anyway (which is quite common due to
application and library probing of the environment) and doesn't leak
information (since both the DAC denial and the MAC denial yield the same
error code).  But if your real concern is capturing the relevant
security contexts upon a DAC denial, we can support that; SELinux
already provides hook functions for copying the contexts of processes
and files to buffers to support the /proc/pid/attr and xattr APIs, so
that audit framework can call those hooks and save that information for
processing by audit_log_exit upon DAC denials as well.

> At this point, my only real concern is that it be easier for users to 
get this
> information and I simply don't know enough about the audit 
infrastructure to
> know whether this will be the case or not. What kind of configuration 
will be
> required to get the additional syscall audit messages?

Boot your kernel with audit=1 or run auditctl -e 1 at any time.  That
enables syscall auditing.  Then, every time SELinux generates an audit
message via avc_audit, a syscall audit record will also be written at
syscall exit, and they can be tied together based on the
timestamp/serial number in the prefix.
 
-- 
Stephen Smalley <sds@tycho.nsa.gov>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov 
with
the words "unsubscribe selinux" without quotes as the message.


[-- Attachment #2: Type: text/html, Size: 4810 bytes --]

RE: dcache_lock deadlock due to auditing

[Karl MacMillan]

> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Thursday, April 14, 2005 12:16 PM
> To: Karl MacMillan
> Cc: 'Serge Hallyn'; selinux@tycho.nsa.gov; 'James Morris'; selinux-
> dev@tresys.com; linux-audit@redhat.com
> Subject: RE: dcache_lock deadlock due to auditing
> 
> On Thu, 2005-04-14 at 12:06 -0400, Karl MacMillan wrote:
> > Just to be clear, this decision is really to continue the movement away from
> avc
> > messages being a complete source for audit message related to SELinux to
> both
> > avc and syscall auditing being required to get a full picture. My sense is
> that
> > this is the direction that things must go in for technical and political
> > reasons, though I have to say that my preference would be (for what it's
> worth)
> > for SELinux avc audit messages to be complete, self-contained, and be
> triggered
> > even with DAC permissions deny access.
> 
> The SELinux avc audit messages will remain complete as far as having all
> the information you need to tie it back to policy or generate policy
> rules, i.e. the security contexts and class information.  The auxiliary
> audit information (pid, exe, path, port, ipaddr, etc) has always just
> been to help track down the causes of denials.  As far as being
> triggered for DAC denials, that just isn't an option, as the LSM hooks
> are placed after the DAC checks in most cases and won't be reached for
> DAC denials.  That avoids filling the logs with MAC denials that would
> have been denied due to DAC anyway (which is quite common due to
> application and library probing of the environment) and doesn't leak
> information (since both the DAC denial and the MAC denial yield the same
> error code).  But if your real concern is capturing the relevant
> security contexts upon a DAC denial, we can support that; SELinux
> already provides hook functions for copying the contexts of processes
> and files to buffers to support the /proc/pid/attr and xattr APIs, so
> that audit framework can call those hooks and save that information for
> processing by audit_log_exit upon DAC denials as well.
> 

Having the context information is part of my real concern. The thought is to
build intrusion detection systems that interpret audit messages in terms of
SELinux policy. The only additional benefit to having all of the data part of
SELinux audit messages is that the policy could control the generation of the
messages - e.g., the intrusion detection piece could change a boolean that
increased the number of audit messages for a particular type. The way this is
moving will cause my audit policy to be stored in two places in vastly different
forms.

Karl

---
Karl MacMillan
Tresys Technology
http://www.tresys.com
(410) 290-1411 ext 134

> 
> --
> Stephen Smalley <sds@tycho.nsa.gov>
> National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: dcache_lock deadlock due to auditing

[serue]

Hi,

Just an update:  the testcases ran fine for 12 hours yesterday with
Stephen's patch.  They had to be stopped due to an unrelated reason, and
have been restarted today.

thanks,
-serge

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: dcache_lock deadlock due to auditing

[Stephen Smalley]

On Fri, 2005-04-15 at 08:09 -0500, serue@us.ibm.com wrote:
> Hi,
> 
> Just an update:  the testcases ran fine for 12 hours yesterday with
> Stephen's patch.  They had to be stopped due to an unrelated reason, and
> have been restarted today.

Good.  Below is an updated version of the same patch against 2.6.12-rc2;
is this consistent with the patch that you have been testing aside from
minor offset adjustments?  Does anyone have any objection to upstreaming
this patch now, as it fixes a real bug and doesn't depend on the rest of
the audit-related patches?

---<snip>---

This patch moves the logging of task-related information (pid, exe,
comm) from the SELinux avc_audit function to audit_log_exit (via an
audit_log_task_info helper) for processing upon syscall exit.  This
eliminates a deadlock on the dcache lock detected during testing at
IBM, since avc_audit can be called from irq or softirq
(file_send_sigiotask, sock_rcv_skb).  It also allows simplification of
the avc_audit code, allows the exe information to be obtained more
reliably, always includes the comm information (useful for scripts),
and avoids including bogus task information for checks performed from
irq or softirq.  Please apply.

--

 kernel/auditsc.c       |   28 ++++++++++++++++++++++++++++
 security/selinux/avc.c |   34 ----------------------------------
 2 files changed, 28 insertions(+), 34 deletions(-)

===== kernel/auditsc.c 1.10 vs edited =====
--- 1.10/kernel/auditsc.c	2005-03-17 11:33:20 -05:00
+++ edited/kernel/auditsc.c	2005-04-15 09:22:15 -04:00
@@ -610,6 +610,33 @@
 		printk(KERN_ERR "audit: freed %d contexts\n", count);
 }
 
+static void audit_log_task_info(struct audit_buffer *ab)
+{
+	char name[sizeof(current->comm)];
+	struct mm_struct *mm = current->mm;
+	struct vm_area_struct *vma;
+
+	get_task_comm(name, current);
+	audit_log_format(ab, " comm=%s", name);
+
+	if (!mm)
+		return;
+
+	down_read(&mm->mmap_sem);
+	vma = mm->mmap;
+	while (vma) {
+		if ((vma->vm_flags & VM_EXECUTABLE) &&
+		    vma->vm_file) {
+			audit_log_d_path(ab, "exe=",
+					 vma->vm_file->f_dentry,
+					 vma->vm_file->f_vfsmnt);
+			break;
+		}
+		vma = vma->vm_next;
+	}
+	up_read(&mm->mmap_sem);
+}
+
 static void audit_log_exit(struct audit_context *context)
 {
 	int i;
@@ -639,6 +666,7 @@
 		  context->gid,
 		  context->euid, context->suid, context->fsuid,
 		  context->egid, context->sgid, context->fsgid);
+	audit_log_task_info(ab);
 	audit_log_end(ab);
 	while (context->aux) {
 		struct audit_aux_data *aux;
===== security/selinux/avc.c 1.23 vs edited =====
--- 1.23/security/selinux/avc.c	2005-03-28 17:21:18 -05:00
+++ edited/security/selinux/avc.c	2005-04-15 09:22:16 -04:00
@@ -532,7 +532,6 @@
                u16 tclass, u32 requested,
                struct av_decision *avd, int result, struct avc_audit_data *a)
 {
-	struct task_struct *tsk = current;
 	struct inode *inode = NULL;
 	u32 denied, audited;
 	struct audit_buffer *ab;
@@ -556,39 +555,6 @@
 	audit_log_format(ab, "avc:  %s ", denied ? "denied" : "granted");
 	avc_dump_av(ab, tclass,audited);
 	audit_log_format(ab, " for ");
-	if (a && a->tsk)
-		tsk = a->tsk;
-	if (tsk && tsk->pid) {
-		struct mm_struct *mm;
-		struct vm_area_struct *vma;
-		audit_log_format(ab, " pid=%d", tsk->pid);
-		if (tsk == current)
-			mm = current->mm;
-		else
-			mm = get_task_mm(tsk);
-		if (mm) {
-			if (down_read_trylock(&mm->mmap_sem)) {
-				vma = mm->mmap;
-				while (vma) {
-					if ((vma->vm_flags & VM_EXECUTABLE) &&
-					    vma->vm_file) {
-						audit_log_d_path(ab, "exe=",
-							vma->vm_file->f_dentry,
-							vma->vm_file->f_vfsmnt);
-						break;
-					}
-					vma = vma->vm_next;
-				}
-				up_read(&mm->mmap_sem);
-			} else {
-				audit_log_format(ab, " comm=%s", tsk->comm);
-			}
-			if (tsk != current)
-				mmput(mm);
-		} else {
-			audit_log_format(ab, " comm=%s", tsk->comm);
-		}
-	}
 	if (a) {
 		switch (a->type) {
 		case AVC_AUDIT_DATA_IPC:

-- 
Stephen Smalley <sds@tycho.nsa.gov>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: dcache_lock deadlock due to auditing

[serue]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> On Fri, 2005-04-15 at 08:09 -0500, serue@us.ibm.com wrote:
> > Hi,
> > 
> > Just an update:  the testcases ran fine for 12 hours yesterday with
> > Stephen's patch.  They had to be stopped due to an unrelated reason, and
> > have been restarted today.
> 
> Good.  Below is an updated version of the same patch against 2.6.12-rc2;
> is this consistent with the patch that you have been testing aside from
> minor offset adjustments?

Yes, patch looks the same.

thanks,
-serge

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: dcache_lock deadlock due to auditing

[Steve G]

>I'm guessing the security context obtained from the SELinux hooks are 
>is represented by a sid. Is this true? 

Yes. This patch is already part of the audit subsystem code and on its way
upstream.

>If that's the case, can it be readily converted to the string representation 
>for display at audit_log_exit time?

The patch does that.

-Steve Grubb


		
__________________________________ 
Do you Yahoo!? 
Yahoo! Small Business - Try our new resources site!
http://smallbusiness.yahoo.com/resources/

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: dcache_lock deadlock due to auditing

[Stephen Smalley]

On Thu, 2005-04-14 at 13:35 -0700, Steve G wrote:
> >I'm guessing the security context obtained from the SELinux hooks are 
> >is represented by a sid. Is this true? 
> 
> Yes. This patch is already part of the audit subsystem code and on its way
> upstream.

I didn't see the original posting, but the security_getprocattr (->
selinux_getprocattr) and security_inode_getsecurity (->
selinux_inode_getsecurity) hooks copy security contexts into buffers
supplied by the caller.  That is what I was referring to.  The pathname
lookup code would need to be modified to invoke
security_inode_getsecurity(), possibly from audit_inode() by passing the
inode structure to it, and copy the context into the auxiliary item list
on the current audit context for display upon audit_log_exit.  And
audit_log_exit could be modified to call security_getprocattr to get the
current process context and display it.  I don't believe anyone has done
that yet.  There has been a patch to log the exe and comm information
for the current task upon audit_log_exit, but that is different.

-- 
Stephen Smalley <sds@tycho.nsa.gov>
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.