Re: [ 105/124] af_unix: dont send SCM_CREDENTIAL when dest socket is NULL

[Eric W. Biederman]

Sven Joachim <svenjoac@xxxxxx> writes:

> On 2013-04-03 00:11 +0200, Greg Kroah-Hartman wrote:
>
>> 3.8-stable review patch.  If anyone has any objections, please let me know.
>
> I'm seeing several complaints from udevd at boot in both 3.8.6-rc1 and
> 3.9-rc5: "udevd[56]: sender uid=65534, message ignored".  Reverting the
> patch below on top of 3.8.6-rc1 fixes that.  I'm using udev version 175
> here, and 65534 is the uid of user "nobody".

Hmm.

Ok.  I don't understand the commit that was being backported here.  I am
pretty certain it a fix for a problem that did not exist.

Unless I am completely mis-reading scm_recv we only generate a
SCM_CREDENTIALS message if the receiving socket asserts SOCK_PASSCRED.
Which means that the only harm that can come from adding scm credentials
to a disconnected af_unix socket is a loss in efficiency.

Not adding scm credentials to be passed to userspace as the commit below
is doing can result is bogus data being passed to userspace.  Which is
very actively WRONG.

Now before scm_recv does anything we first call scm_set_cred.  If no
credential was passed to scm_set_cred we set the uid to INVALID_UID.
Which scm_recv in the call from_kuid_munged translates into 65534 for
reporting to userspace.

So this is is pretty clearly a case of us not sending the unix
credentials.

Since not sending credential is just a performance optimization I can
see no earthly reason why the commit below should have been applied in
the first place, and no reason why it should have been backported in the
second place.  So my vote is that we revert this bogus commit.  Upstream
and then backport the revert.

Am I missing something?

Eric

>> From: dingtianhong <dingtianhong@xxxxxxxxxx>
>>
>> [ Upstream commit 14134f6584212d585b310ce95428014b653dfaf6 ]
>>
>> SCM_SCREDENTIALS should apply to write() syscalls only either source or destination
>> socket asserted SOCK_PASSCRED. The original implememtation in maybe_add_creds is wrong,
>> and breaks several LSB testcases ( i.e. /tset/LSB.os/netowkr/recvfrom/T.recvfrom).
>>
>> Origionally-authored-by: Karel Srot <ksrot@xxxxxxxxxx>
>> Signed-off-by: Ding Tianhong <dingtianhong@xxxxxxxxxx>
>> Acked-by: Eric Dumazet <edumazet@xxxxxxxxxx>
>> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx>
>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>>  net/unix/af_unix.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1414,8 +1414,8 @@ static void maybe_add_creds(struct sk_bu
>>  	if (UNIXCB(skb).cred)
>>  		return;
>>  	if (test_bit(SOCK_PASSCRED, &sock->flags) ||
>> -	    !other->sk_socket ||
>> -	    test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) {
>> +	    (other->sk_socket &&
>> +	    test_bit(SOCK_PASSCRED, &other->sk_socket->flags))) {
>>  		UNIXCB(skb).pid  = get_pid(task_tgid(current));
>>  		UNIXCB(skb).cred = get_current_cred();
>>  	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/