[PATCH] af_unix: Only allow recv on connected seqpacket sockets.

[PATCH] af_unix: Only allow recv on connected seqpacket sockets.

[Eric W. Biederman]

This fixes the following oops discovered by Dan Aloni:
> Anyway, the following is the output of the Oops that I got on the
> Ubuntu kernel on which I first detected the problem
> (2.6.37-12-generic). The Oops that followed will be more useful, I
> guess.

>[ 5594.669852] BUG: unable to handle kernel NULL pointer dereference
> at           (null)
> [ 5594.681606] IP: [<ffffffff81550b7b>] unix_dgram_recvmsg+0x1fb/0x420
> [ 5594.687576] PGD 2a05d067 PUD 2b951067 PMD 0
> [ 5594.693720] Oops: 0002 [#1] SMP
> [ 5594.699888] last sysfs file:

The bug was that unix domain sockets use a pseduo packet for
connecting and accept uses that psudo packet to get the socket.
In the buggy seqpacket case we were allowing unconnected
sockets to call recvmsg and try to receive the pseudo packet.

That is always wrong and as of commit 7361c36c5 the pseudo
packet had become enough different from a normal packet
that the kernel started oopsing.

Do for seqpacket_recv what was done for seqpacket_send in 2.5
and only allow it on connected seqpacket sockets.

Cc: stable@kernel.org
Tested-by: Dan Aloni <dan@aloni.org>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 net/unix/af_unix.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 3a43a83..b1d75be 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -524,6 +524,8 @@ static int unix_dgram_connect(struct socket *, struct sockaddr *,
 			      int, int);
 static int unix_seqpacket_sendmsg(struct kiocb *, struct socket *,
 				  struct msghdr *, size_t);
+static int unix_seqpacket_recvmsg(struct kiocb *, struct socket *,
+				  struct msghdr *, size_t, int);
 
 static const struct proto_ops unix_stream_ops = {
 	.family =	PF_UNIX,
@@ -583,7 +585,7 @@ static const struct proto_ops unix_seqpacket_ops = {
 	.setsockopt =	sock_no_setsockopt,
 	.getsockopt =	sock_no_getsockopt,
 	.sendmsg =	unix_seqpacket_sendmsg,
-	.recvmsg =	unix_dgram_recvmsg,
+	.recvmsg =	unix_seqpacket_recvmsg,
 	.mmap =		sock_no_mmap,
 	.sendpage =	sock_no_sendpage,
 };
@@ -1699,6 +1701,18 @@ static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock,
 	return unix_dgram_sendmsg(kiocb, sock, msg, len);
 }
 
+static int unix_seqpacket_recvmsg(struct kiocb *iocb, struct socket *sock,
+			      struct msghdr *msg, size_t size,
+			      int flags)
+{
+	struct sock *sk = sock->sk;
+
+	if (sk->sk_state != TCP_ESTABLISHED)
+		return -ENOTCONN;
+
+	return unix_dgram_recvmsg(iocb, sock, msg, size, flags);
+}
+
 static void unix_copy_addr(struct msghdr *msg, struct sock *sk)
 {
 	struct unix_sock *u = unix_sk(sk);
-- 
1.6.5.2.143.g8cc62

Re: [PATCH] af_unix: Only allow recv on connected seqpacket sockets.

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Sun, 24 Apr 2011 04:54:57 -0700

> +static int unix_seqpacket_recvmsg(struct kiocb *iocb, struct socket *sock,
> +			      struct msghdr *msg, size_t size,
> +			      int flags)
> +{
> +	struct sock *sk = sock->sk;
> +
> +	if (sk->sk_state != TCP_ESTABLISHED)
> +		return -ENOTCONN;

As for unix_seqpacket_sendmsg(), you need to add a check for sock_error()
or similar here otherwise -ECONNRESET is not reported correctly.

In fact, recvmsg() is even harder than sendmsg() to handle correctly,
because we have to also properly report EOF on seqpacket sockets which
have RCV_SHUTDOWN set.

So a lot more work has to go into this change to make it fix the bug
without also breaking existing semantics.

Anyways, see:

commit 6e14891f4d16f8a9e0bc3a8408f73b3aed93ab0a
Author: James Morris <jmorris@redhat.com>
Date:   Fri Nov 19 07:02:41 2004 -0800

    [AF_UNIX]: Don't lose ECONNRESET in unix_seqpacket_sendmsg()
    
    The fix for SELinux w/SOCK_SEQPACKET had an error,
    noted by Alan Cox.  This fixes it.
    
    Signed-off-by: James Morris <jmorris@redhat.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 16faa9d..8902c4a 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1513,13 +1513,18 @@ out_err:
 static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock,
 				  struct msghdr *msg, size_t len)
 {
+	int err;
 	struct sock *sk = sock->sk;
 	
+	err = sock_error(sk);
+	if (err)
+		return err;
+
 	if (sk->sk_state != TCP_ESTABLISHED)
 		return -ENOTCONN;
 
-	if (msg->msg_name || msg->msg_namelen)
-		return -EINVAL;
+	if (msg->msg_namelen)
+		msg->msg_namelen = 0;
 
 	return unix_dgram_sendmsg(kiocb, sock, msg, len);
 }

Re: [PATCH] af_unix: Only allow recv on connected seqpacket sockets.

[Eric W. Biederman]

David Miller <davem@davemloft.net> writes:

> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Sun, 24 Apr 2011 04:54:57 -0700
>
>> +static int unix_seqpacket_recvmsg(struct kiocb *iocb, struct socket *sock,
>> +			      struct msghdr *msg, size_t size,
>> +			      int flags)
>> +{
>> +	struct sock *sk = sock->sk;
>> +
>> +	if (sk->sk_state != TCP_ESTABLISHED)
>> +		return -ENOTCONN;
>
> As for unix_seqpacket_sendmsg(), you need to add a check for sock_error()
> or similar here otherwise -ECONNRESET is not reported correctly.
>
> In fact, recvmsg() is even harder than sendmsg() to handle correctly,
> because we have to also properly report EOF on seqpacket sockets which
> have RCV_SHUTDOWN set.
>
> So a lot more work has to go into this change to make it fix the bug
> without also breaking existing semantics.

Really?

When I read through the code I am failing to see the issues you are
seeing.

When the other socket in an established connection calls unix_shutdown
or unix_release_sock.  sk->sk_shutdown is changed, but sk_state is
left at TCP_ESTABLISHED.  Therefore we do not need a special
case in unix_seqpacket_recvmsg to handle the RCV_SHUTDOWN case
because in any case where that applies we will be in TCP_ESTABLISHED
and we will simply call unix_dgram_recvmsg.

As for ECONNRESET when I look a look at the code it appears to be
another variant of the other side calling shutdown or close.   So if
it applies we should remain in TCP_ESTABLISHED, and
unix_seqpacket_recvmsg should not need to do anything.

So looking at this the only times I can see that sk_state would
not be TCP_ESTABLISHED in a unix domain seqpacket socket are.
- On a listening socket, where calling recvmsg is what this
  patch is meant to address.
- Before we call connect or listen.
  Which appears to be equally broken today.  The only errors
  I can see happening in the case we are not connected today
  are blocking forever or returning -EINTR if we timeout.

Adding sock_error() handling into the new unix_seqpacket_recvmsg makes a
fair amount of sense but adding a new call to sock_error in that path
seems marginally more likely to change error codes and break existing
apps.  We already have a few other unconditional error codes before
we check sk_err in unix_dgram_recvmsg. 


> Anyways, see:
>
> commit 6e14891f4d16f8a9e0bc3a8408f73b3aed93ab0a
> Author: James Morris <jmorris@redhat.com>
> Date:   Fri Nov 19 07:02:41 2004 -0800
>
>     [AF_UNIX]: Don't lose ECONNRESET in unix_seqpacket_sendmsg()
>     
>     The fix for SELinux w/SOCK_SEQPACKET had an error,
>     noted by Alan Cox.  This fixes it.
>     
>     Signed-off-by: James Morris <jmorris@redhat.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>

Looking into it.  That patch appears to have been unnecessary.
We never transition out of the state TCP_ESTABLISHED once we get
there, and we can never get ECONNRESET unless we are connected.

Arguably we could reduce unix_seqpacket_sendmsg to simply 

static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock,
  				  struct msghdr *msg, size_t len)
{
	if (msg->msgnamelen)
        	msg->msgnamelen = 0;
        return unix_dgram_sendmsg(kiocb, sock, msg, len);
}

But I think having the explicit TCP_ESTABLISHED check makes for better
maintainability, of unix_dgram_sendmesg.

So having gone through all of that it looks like my patch needs a
comment saying that once we are in TCP_ESTABLISHED we cannot leave,
and that nothing can happen before we are TCP_ESTABLISHED.

We can use sock_error to check sk_err, as it seems good hygiene
but it also appears pointless.  Especially for recvmsg where ECONNRESET
never applies.

Eric


> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 16faa9d..8902c4a 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1513,13 +1513,18 @@ out_err:
>  static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock,
>  				  struct msghdr *msg, size_t len)
>  {
> +	int err;
>  	struct sock *sk = sock->sk;
>  	
> +	err = sock_error(sk);
> +	if (err)
> +		return err;
> +
>  	if (sk->sk_state != TCP_ESTABLISHED)
>  		return -ENOTCONN;
>  
> -	if (msg->msg_name || msg->msg_namelen)
> -		return -EINVAL;
> +	if (msg->msg_namelen)
> +		msg->msg_namelen = 0;
>  
>  	return unix_dgram_sendmsg(kiocb, sock, msg, len);
>  }

Re: [PATCH] af_unix: Only allow recv on connected seqpacket sockets.

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Mon, 25 Apr 2011 07:26:27 -0700

> When the other socket in an established connection calls unix_shutdown
> or unix_release_sock.  sk->sk_shutdown is changed, but sk_state is
> left at TCP_ESTABLISHED.  Therefore we do not need a special
> case in unix_seqpacket_recvmsg to handle the RCV_SHUTDOWN case
> because in any case where that applies we will be in TCP_ESTABLISHED
> and we will simply call unix_dgram_recvmsg.

Thanks for explaining Eric, patch applied.