Message-ID: <m1sjprfryg.fsf@fess.ebiederm.org>
Date:	Wed, 27 Jul 2011 12:23:35 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Tim Gardner <tim.gardner@...onical.com>
Cc:	Andi Kleen <andi@...stfloor.org>, dan@...ni.org,
	davem@...emloft.net, gregkh@...e.de, ak@...ux.intel.com,
	linux-kernel@...r.kernel.org, stable@...nel.org,
	tim.bird@...sony.com, stable@...r.kernel.org
Subject: Re: [stable] [PATCH] [26/98] af_unix: Only allow recv on connected seqpacket sockets.

Tim Gardner <tim.gardner@...onical.com> writes:

> 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@...ssion.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@...ni.org>
>> Signed-off-by: Eric W. Biederman<ebiederm@...ssion.com>
>> Signed-off-by: David S. Miller<davem@...emloft.net>
>> Signed-off-by: Greg Kroah-Hartman<gregkh@...e.de>
>> Signed-off-by: Andi Kleen<ak@...ux.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@...ux.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.

No.  This patch is no more likely to have caused your network regression
than any other patch you added to your kernel at that time.  Touching
af_unix.c in a trivial way will not affect tcp/ip networking.

I am insulted that you said this change caused your problem rather than
saying it was simply correlated with your problem.

> 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.

That you did not find the original bug report I find as dubious as the
rest of your analysis of this patch.


The only kernels that can oops in the way that was reported are kernels
that have the fix to the make it safe to pass credentials between
processes in different user or pid namespaces.

My patch only affects attempting to send or recv packets on an af_unix
after calling listen, where accept is the only valid operation.  Only
broken application can possibly care about this case.

For 2.6.32 you are not open to the chance of an oops.  So shrug who
cares if you defend against it.

For 2.6.38, and Andi's 2.6.35 which has the potential for an
unprivileged process to trigger an oops, it seems irresponsible to me to
not include this change.  People who exploit kernel flaws seem good at
taking random Oops's and converting them into methods for privilege
escalation.

Tim your argument against the patch feels like an argument that we
should revert this patch from the networking stack in Linus's kernel.
Is that what you were trying to say?

Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists