Gaah: selinux_socket_unix_stream_connect oops

Gaah: selinux_socket_unix_stream_connect oops

[Linus Torvalds]

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

This was actually a regression entry, but only ever reported once by
Jeremy, I think. So it was basically ignored as not being very common
and there not being any hints about what causes it.

But after doing the 2.6.37 release, and intending to put it on all the
machines I have access to, guess what I find on the kids computer?
Right.

It must be a reasonably rare race condition, because that computer had
been up for three weeks or so (since middle of December), but
yesterday evening it crashed due to that thing.

The code disassembly is

  13:	55                   	push   %ebp
  14:	89 e5                	mov    %esp,%ebp
  16:	57                   	push   %edi
  17:	8d 7d 90             	lea    -0x70(%ebp),%edi
  1a:	56                   	push   %esi
  1b:	53                   	push   %ebx
  1c:	83 ec 6c             	sub    $0x6c,%esp
  1f:	8b 40 14             	mov    0x14(%eax),%eax
  22:	8b 52 14             	mov    0x14(%edx),%edx
  25:	8b 98 58 01 00 00    	mov    0x158(%eax),%ebx
  2b:*	8b 82 58 01 00 00    	mov    0x158(%edx),%eax     <-- trapping
instruction
  31:	89 45 8c             	mov    %eax,-0x74(%ebp)
  34:	31 c0                	xor    %eax,%eax
  36:	8b b1 58 01 00 00    	mov    0x158(%ecx),%esi
  3c:	89 7d 88             	mov    %edi,-0x78(%ebp)

which means that it's "other->sk" that is NULL, which I think matches
Jeremy's case exactly.

The logs have a hint: this seems to have coincided with the
console-kit-daemon giving a warning like:

  WARNING: Couldn't read /proc/13585/environ: Failed to open file
'/proc/13585/environ': No such file or directory

and then NetworkManager having a bunch of authentication warnings that
end up about being

  Could not get UID of name ':1.3871': no such name

(full text in the attachment).

So I wonder if there is some subtle race that happens when one end of
a unix domain socket attaches just as another end disconnects?
Especially as "security_unix_stream_connect()" is called before the
whole connect sequence is really final. It's generally
"unix_release()" that sets 'sock->sk' to NULL.

Btw, why do we pass in "sock" and "other->sk_socket" ("struct
socket"), when it appears that what the security code really wants to
get "struct sock" (which would be "sk" and "other" in the caller)? The
calling convention seems to result in (a) this NULL pointer thing and
(b) all these extra dereferences.

Comments? Ideas?

                              Linus

[-- Attachment #2: kids.txt --]
[-- Type: text/plain, Size: 5215 bytes --]

Happened with

 Linux version 2.6.37-rc5-00333-gdaefc3d

after perhaps three weeks of uptime.  That kernel isn't in git, it has
an extra patch (to force-enable AHCI on the mac mini), so it's really
v2.6.37-rc5-332-g0fcdcfbbc98f in baseline.

---
Jan  4 17:02:50 kids console-kit-daemon[2884]: WARNING: Couldn't read /proc/13585/environ: Failed to open file '/proc/13585/environ': No such file or directory
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> error requesting auth for org.freedesktop.NetworkManager.network-control: (6) Remote Exception invoking org.freedesktop.PolicyKit1.Authority.CheckAuthorization() on /org/freede...
				...NameHasNoOwner: Could not get UID of name ':1.3871': no such name
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> User connections unavailable: (6) Remote Exception invoking org.freedesktop.PolicyKit1.Authority.CheckAuthorization() ...
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> error requesting auth for org.freedesktop.NetworkManager.enable-disable-network: (6) Remote Exception invoking org.fre...
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> error requesting auth for org.freedesktop.NetworkManager.sleep-wake: (6) Remote Exception invoking org.freedesktop.Pol...
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> error requesting auth for org.freedesktop.NetworkManager.enable-disable-wifi: (6) Remote Exception invoking org.freede...
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> error requesting auth for org.freedesktop.NetworkManager.enable-disable-wwan: (6) Remote Exception invoking org.freede...
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> error requesting auth for org.freedesktop.NetworkManager.use-user-connections: (6) Remote Exception invoking org.freed...
Jan  4 17:02:50 kids NetworkManager[2438]: <warn> error requesting auth for org.freedesktop.NetworkManager.network-control: (6) Remote Exception invoking org.freedeskto...
Jan  4 17:02:50 kids bonobo-activation-server (celeste-15075): could not associate with desktop session: Failed to connect to socket /tmp/dbus-8AXjzuuw2I: Connection re...
				...NameHasNoOwner: Could not get UID of name ':1.3871': no such name
Jan  4 17:02:50 kids kernel: [1755465.955480] BUG: unable to handle kernel NULL pointer dereference at 00000158
Jan  4 17:02:50 kids kernel: [1755465.956226] IP: [<c111297e>] selinux_socket_unix_stream_connect+0x18/0x84
Jan  4 17:02:50 kids kernel: [1755465.956226] *pde = 00000000
Jan  4 17:02:50 kids kernel: [1755465.956226] Oops: 0000 [#1] SMP
Jan  4 17:02:50 kids kernel: [1755465.956226] last sysfs file: /sys/devices/virtual/sound/timer/uevent
Jan  4 17:02:50 kids kernel: [1755465.956226] Modules linked in: [last unloaded: scsi_wait_scan]
Jan  4 17:02:50 kids kernel: [1755465.956226]
Jan  4 17:02:50 kids kernel: [1755465.956226] Pid: 15075, comm: bonobo-activati Not tainted 2.6.37-rc5-00333-gdaefc3d #13 Mac-F4208EC8/Macmini1,1
Jan  4 17:02:50 kids kernel: [1755465.956226] EIP: 0060:[<c111297e>] EFLAGS: 00210296 CPU: 1
Jan  4 17:02:50 kids kernel: [1755465.956226] EIP is at selinux_socket_unix_stream_connect+0x18/0x84
Jan  4 17:02:50 kids kernel: [1755465.956226] EAX: ee48b200 EBX: f1f983c0 ECX: ee48be00 EDX: 00000000
Jan  4 17:02:50 kids kernel: [1755465.956226] ESI: ee48b200 EDI: f59c5e1c EBP: f59c5e8c ESP: f59c5e14
Jan  4 17:02:50 kids kernel: [1755465.956226]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Jan  4 17:02:50 kids kernel: [1755465.956226] Process bonobo-activati (pid: 15075, ti=f59c4000 task=ee45eb50 task.ti=f59c4000)
Jan  4 17:02:50 kids kernel: [1755465.956226] Stack:
Jan  4 17:02:50 kids kernel: [1755465.956226]  00000000 00000000 00000000 00000000 00000000 00200046 00200046 00000059
Jan  4 17:02:50 kids kernel: [1755465.956226]  00000013 00000000 f59c5e44 c103070e f59c5e5c c1003cc5 f59c5e64 ee432a00
Jan  4 17:02:50 kids kernel: [1755465.956226]  ee48b200 ee48be00 f59c5ed4 c1002ce9 ee432a00 f59c5ec0 e1b02900 ee48b200
Jan  4 17:02:50 kids kernel: [1755465.956226] Call Trace:
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c103070e>] ? irq_exit+0x39/0x5d
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c1003cc5>] ? do_IRQ+0x83/0x97
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c1002ce9>] ? common_interrupt+0x29/0x30
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c111072c>] ? security_unix_stream_connect+0x10/0x13
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c1390029>] ? unix_stream_connect+0x1e3/0x35e
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c1316047>] ? sys_connect+0x60/0x7d
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c1316b29>] ? sys_socketcall+0x8f/0x1a5
Jan  4 17:02:50 kids kernel: [1755465.956226]  [<c100278c>] ? sysenter_do_call+0x12/0x22
Jan  4 17:02:50 kids kernel: [1755465.956226] Code: 56 68 00 00 04 00 e8 ba f2 ff ff 8d 65 f4 5b 5e 5f c9 c3 55 89 e5 57 8d 7d 90 56 53 83 ec 6c 8b 40 14 8b 52 14 8b 98 58 01 00 00 <8b> 82 58 01 00 00 89 45 8c 31 c0 8b b1 58 01 00 00 89 7d 88 b9
Jan  4 17:02:50 kids kernel: [1755465.956226] EIP: [<c111297e>] selinux_socket_unix_stream_connect+0x18/0x84 SS:ESP 0068:f59c5e14
Jan  4 17:02:50 kids kernel: [1755465.956226] CR2: 0000000000000158
Jan  4 17:02:50 kids kernel: [1755466.037178] ---[ end trace 9fd0d9b8feb78e69 ]---

Re: Gaah: selinux_socket_unix_stream_connect oops

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 5 Jan 2011 08:27:01 -0800

> So I wonder if there is some subtle race that happens when one end of
> a unix domain socket attaches just as another end disconnects?
> Especially as "security_unix_stream_connect()" is called before the
> whole connect sequence is really final. It's generally
> "unix_release()" that sets 'sock->sk' to NULL.
> 
> Btw, why do we pass in "sock" and "other->sk_socket" ("struct
> socket"), when it appears that what the security code really wants to
> get "struct sock" (which would be "sk" and "other" in the caller)? The
> calling convention seems to result in (a) this NULL pointer thing and
> (b) all these extra dereferences.
> 
> Comments? Ideas?

There is nothing which blocks that unix_release() code which sets
socket->sk to NULL, it runs asynchronously to this connect code.

The reason it passes in the socket pointer instead of the pointer to
struct sock is because that's what SMACK wants to use, as it needs to
get at SOCK_INODE().

See, even you think the whole world is SELINUX :-)))

More seriously, we can get at the struct socket via sk->sk_socket in
the SMACK code.  sk->sk_socket, unlike socket->sk, has it's state
change to NULL (via sock_orphen()) protected by unix_state_lock(),
which we hold for "other" in this unix connect code path.

Therefore I propose we fix this like so:

--------------------
af_unix: Avoid socket->sk NULL OOPS in stream connect security hooks.

unix_release() can asynchornously set socket->sk to NULL, and
it does so without holding the unix_state_lock() on "other"
during stream connects.

However, the reverse mapping, sk->sk_socket, is only transitioned
to NULL under the unix_state_lock().

Therefore make the security hooks follow the reverse mapping instead
of the forward mapping.

Reported-by: Jeremy Fitzhardinge <jeremy@goop.org>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/security.h b/include/linux/security.h
index fd4d55f..d47a4c2 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -796,8 +796,9 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  * @unix_stream_connect:
  *	Check permissions before establishing a Unix domain stream connection
  *	between @sock and @other.
- *	@sock contains the socket structure.
- *	@other contains the peer socket structure.
+ *	@sock contains the sock structure.
+ *	@other contains the peer sock structure.
+ *	@newsk contains the new sock structure.
  *	Return 0 if permission is granted.
  * @unix_may_send:
  *	Check permissions before connecting or sending datagrams from @sock to
@@ -1568,8 +1569,7 @@ struct security_operations {
 	int (*inode_getsecctx)(struct inode *inode, void **ctx, u32 *ctxlen);
 
 #ifdef CONFIG_SECURITY_NETWORK
-	int (*unix_stream_connect) (struct socket *sock,
-				    struct socket *other, struct sock *newsk);
+	int (*unix_stream_connect) (struct sock *sock, struct sock *other, struct sock *newsk);
 	int (*unix_may_send) (struct socket *sock, struct socket *other);
 
 	int (*socket_create) (int family, int type, int protocol, int kern);
@@ -2525,8 +2525,7 @@ static inline int security_inode_getsecctx(struct inode *inode, void **ctx, u32
 
 #ifdef CONFIG_SECURITY_NETWORK
 
-int security_unix_stream_connect(struct socket *sock, struct socket *other,
-				 struct sock *newsk);
+int security_unix_stream_connect(struct sock *sock, struct sock *other, struct sock *newsk);
 int security_unix_may_send(struct socket *sock,  struct socket *other);
 int security_socket_create(int family, int type, int protocol, int kern);
 int security_socket_post_create(struct socket *sock, int family,
@@ -2567,8 +2566,8 @@ void security_tun_dev_post_create(struct sock *sk);
 int security_tun_dev_attach(struct sock *sk);
 
 #else	/* CONFIG_SECURITY_NETWORK */
-static inline int security_unix_stream_connect(struct socket *sock,
-					       struct socket *other,
+static inline int security_unix_stream_connect(struct sock *sock,
+					       struct sock *other,
 					       struct sock *newsk)
 {
 	return 0;
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 417d7a6..dd419d2 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1157,7 +1157,7 @@ restart:
 		goto restart;
 	}
 
-	err = security_unix_stream_connect(sock, other->sk_socket, newsk);
+	err = security_unix_stream_connect(sk, other, newsk);
 	if (err) {
 		unix_state_unlock(sk);
 		goto out_unlock;
diff --git a/security/capability.c b/security/capability.c
index c773635..2a5df2b 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -548,7 +548,7 @@ static int cap_sem_semop(struct sem_array *sma, struct sembuf *sops,
 }
 
 #ifdef CONFIG_SECURITY_NETWORK
-static int cap_unix_stream_connect(struct socket *sock, struct socket *other,
+static int cap_unix_stream_connect(struct sock *sock, struct sock *other,
 				   struct sock *newsk)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 1b798d3..e5fb07a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -977,8 +977,7 @@ EXPORT_SYMBOL(security_inode_getsecctx);
 
 #ifdef CONFIG_SECURITY_NETWORK
 
-int security_unix_stream_connect(struct socket *sock, struct socket *other,
-				 struct sock *newsk)
+int security_unix_stream_connect(struct sock *sock, struct sock *other, struct sock *newsk)
 {
 	return security_ops->unix_stream_connect(sock, other, newsk);
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c82538a..6f637d2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3921,18 +3921,18 @@ static int selinux_socket_shutdown(struct socket *sock, int how)
 	return sock_has_perm(current, sock->sk, SOCKET__SHUTDOWN);
 }
 
-static int selinux_socket_unix_stream_connect(struct socket *sock,
-					      struct socket *other,
+static int selinux_socket_unix_stream_connect(struct sock *sock,
+					      struct sock *other,
 					      struct sock *newsk)
 {
-	struct sk_security_struct *sksec_sock = sock->sk->sk_security;
-	struct sk_security_struct *sksec_other = other->sk->sk_security;
+	struct sk_security_struct *sksec_sock = sock->sk_security;
+	struct sk_security_struct *sksec_other = other->sk_security;
 	struct sk_security_struct *sksec_new = newsk->sk_security;
 	struct common_audit_data ad;
 	int err;
 
 	COMMON_AUDIT_DATA_INIT(&ad, NET);
-	ad.u.net.sk = other->sk;
+	ad.u.net.sk = other;
 
 	err = avc_has_perm(sksec_sock->sid, sksec_other->sid,
 			   sksec_other->sclass,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 489a85a..ccb71a0 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2408,22 +2408,22 @@ static int smack_setprocattr(struct task_struct *p, char *name,
 
 /**
  * smack_unix_stream_connect - Smack access on UDS
- * @sock: one socket
- * @other: the other socket
+ * @sock: one sock
+ * @other: the other sock
  * @newsk: unused
  *
  * Return 0 if a subject with the smack of sock could access
  * an object with the smack of other, otherwise an error code
  */
-static int smack_unix_stream_connect(struct socket *sock,
-				     struct socket *other, struct sock *newsk)
+static int smack_unix_stream_connect(struct sock *sock,
+				     struct sock *other, struct sock *newsk)
 {
-	struct inode *sp = SOCK_INODE(sock);
-	struct inode *op = SOCK_INODE(other);
+	struct inode *sp = SOCK_INODE(sock->sk_socket);
+	struct inode *op = SOCK_INODE(other->sk_socket);
 	struct smk_audit_info ad;
 
 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_NET);
-	smk_ad_setfield_u_net_sk(&ad, other->sk);
+	smk_ad_setfield_u_net_sk(&ad, other);
 	return smk_access(smk_of_inode(sp), smk_of_inode(op),
 				 MAY_READWRITE, &ad);
 }

Re: Gaah: selinux_socket_unix_stream_connect oops

[Linus Torvalds]

On Wed, Jan 5, 2011 at 2:25 PM, David Miller <davem@davemloft.net> wrote:
>
> More seriously, we can get at the struct socket via sk->sk_socket in
> the SMACK code.  sk->sk_socket, unlike socket->sk, has it's state
> change to NULL (via sock_orphen()) protected by unix_state_lock(),
> which we hold for "other" in this unix connect code path.
>
> Therefore I propose we fix this like so:

Looks fine to me.

And no, I don't think that selinux is all the world, but selinux is
the _common_ case, and with the cross-pointers, the only difference
can be whether you need to dereference the pointer or not - so
choosing the "extra dereference" case for the common case seems silly.

The fact that this also fixes locking is obviously an even better
reason to do it, though ;)

                      Linus

Re: Gaah: selinux_socket_unix_stream_connect oops

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 5 Jan 2011 15:32:44 -0800

> On Wed, Jan 5, 2011 at 2:25 PM, David Miller <davem@davemloft.net> wrote:
>>
>> More seriously, we can get at the struct socket via sk->sk_socket in
>> the SMACK code.  sk->sk_socket, unlike socket->sk, has it's state
>> change to NULL (via sock_orphen()) protected by unix_state_lock(),
>> which we hold for "other" in this unix connect code path.
>>
>> Therefore I propose we fix this like so:
> 
> Looks fine to me.
> 
> And no, I don't think that selinux is all the world, but selinux is
> the _common_ case, and with the cross-pointers, the only difference
> can be whether you need to dereference the pointer or not - so
> choosing the "extra dereference" case for the common case seems silly.
> 
> The fact that this also fixes locking is obviously an even better
> reason to do it, though ;)

Right :)

I'll toss this your way during the merge window and queue it up for
later -stable submission as well.

Thanks.