From: ebiederm@xmission.com (Eric W. Biederman)
To: Andy Lutomirski <luto@amacapital.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>, Sven Joachim <svenjoac@gmx.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        stable@vger.kernel.org, Ding Tianhong <dingtianhong@huawei.com>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>
References: <20130402221104.163133110@linuxfoundation.org>
	<20130402221116.307254752@linuxfoundation.org>
	<87vc833kpf.fsf@turtle.gmx.de>
	<1364997630.13853.19.camel@edumazet-glaptop>
	<87a9pfzm33.fsf@turtle.gmx.de>
	<1365003300.13853.25.camel@edumazet-glaptop> <515C6DCD.50902@mit.edu>
	<1365014637.13853.36.camel@edumazet-glaptop>
	<CALCETrVWV=HEv7q5AUYeCjZOq52omz8moYTwympB6_G4Bz7p8Q@mail.gmail.com>
Date: Wed, 03 Apr 2013 17:14:31 -0700
In-Reply-To: <CALCETrVWV=HEv7q5AUYeCjZOq52omz8moYTwympB6_G4Bz7p8Q@mail.gmail.com>
	(Andy Lutomirski's message of "Wed, 3 Apr 2013 12:01:36 -0700")
Message-ID: <87a9pfm9so.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2685
Lines: 70

Andy Lutomirski <luto@amacapital.net> writes:

> On Wed, Apr 3, 2013 at 11:43 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Wed, 2013-04-03 at 10:58 -0700, Andy Lutomirski wrote:
>>
>>>
>>> This sounds suspiciously like an SCM_CREDENTIALS bug triggered by a
>>> race.  There's a fix (that needs both a new version from me and a review
>>> by someone) here:
>>>
>>> http://www.spinics.net/lists/netdev/msg229948.html
>>
>> Hmm... this is not a stable candidate, IMHO.
>
> Agreed.
>
>>
>> This has to be fixed (if needed) in a more easy way.
>>
>> What about this one liner ?
>>
>> CC Eric W. Biederman  as he wrote commit
>> dbe9a4173ea53b72b2c3
>> (scm: Don't use struct ucred in NETLINK_CB and struct scm_cookie.)
>>
>> diff --git a/include/net/scm.h b/include/net/scm.h
>> index 975cca0..42359d8 100644
>> --- a/include/net/scm.h
>> +++ b/include/net/scm.h
>> @@ -120,7 +120,7 @@ static __inline__ void scm_recv(struct socket *sock, struct msghdr *msg,
>>                 return;
>>         }
>>
>> -       if (test_bit(SOCK_PASSCRED, &sock->flags)) {
>> +       if (test_bit(SOCK_PASSCRED, &sock->flags) && scm->creds.pid) {
>>                 struct user_namespace *current_ns = current_user_ns();
>>                 struct ucred ucreds = {
>>                         .pid = scm->creds.pid,
>>
>>
>
> That looks like it's correct.  If it gets applied, I'll respin my
> patches on top of it.
>
> (This approach may be a POSIX violation for all I know, and it's even
> possible that some really fragile userspace breaks.  But I doubt it,
> and anything that will break as a result is already operating in a
> highly confused state; hence the original problem.)

It certainly looks like we are not giving userspace what userspace asked
for, which can break in all kinds of subtle ways.  And I can't possibly
see how not giving udev any information will when udev asked for the
sender will fix anything.

I think we need to answer why in the world do we do not want to pass
credentials from an unconnected unix mode socket, before we ask
why don't we want to deliver credentials that we didn't pass when
passing of credentials was explicitly requested.

If the only concern about the LSB test case is performance I think we
need to revert the original commit and just stop passing a struct cred
pointer.  If there is a concern about the data I think we need a better
explanation of what those LSB test cases were that broke.

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