Re: [stable] [PATCH] [26/98] af_unix: Only allow recv on connected seqpacket sockets.

Tim Gardner
2011-07-27T15:59:22Z

On 07/26/2011 06:35 PM, Andi Kleen wrote:
> 2.6.35-longterm review patch. If anyone has any objections, please let me know.
>
> ------------------
> From: Eric W. Biederman<ebiederm@xmission.com>
>
> commit a05d2ad1c1f391c7f514a1d1e09b5417968a7d07 upstream.
>
> 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.
>
> Tested-by: Dan Aloni<dan@aloni.org>
> Signed-off-by: Eric W. Biederman<ebiederm@xmission.com>
> Signed-off-by: David S. Miller<davem@davemloft.net>
> Signed-off-by: Greg Kroah-Hartman<gregkh@suse.de>
> Signed-off-by: Andi Kleen<ak@linux.intel.com>
>
> ---
> net/unix/af_unix.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.35.y/net/unix/af_unix.c
> ===================================================================
> --- linux-2.6.35.y.orig/net/unix/af_unix.c
> +++ linux-2.6.35.y/net/unix/af_unix.c
> @@ -504,6 +504,8 @@ static int unix_dgram_connect(struct soc
> 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,
> @@ -563,7 +565,7 @@ static const struct proto_ops unix_seqpa
> .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,
> };
> @@ -1676,6 +1678,18 @@ static int unix_seqpacket_sendmsg(struct
> 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);
>
>
>
>
> _______________________________________________
> stable mailing list
> stable@linux.kernel.org
> http://linux.kernel.org/mailman/listinfo/stable

Andi - Ubuntu has reverted this patch for both Lucid (2.6.32) and Natty
(2.6.38) as it appears to cause a networking regression, though we never
really figured out root cause. Empirically, reverting the patch solved
the issue. Eric Biederman theorized that it uncovered a user space issue
(trying to read before listen), but the bug reporter seems to have lost
interest in testing kernels so we couldn't ever pin it down.

http://bugs.launchpad.net/bugs/791512

We also decided that reverting this patch was likely OK because we
couldn't find any Launchpad reports of the Ooops mentioned in the patch
commit log, nor does a google search turn up any hits.

rtg
--
Tim Gardner tim.gardner@canonical.com