af_unix: dont send SCM_CREDENTIAL when dest socket is NULL

PATCH Superseded dingtianhong 3 participants 8 messages Mar 25, 2013 – Mar 27, 2013

Subsystems (3)

Networking Network drivers Networking unix sockets

lore

Share & tools

lore

dingtianhong

author

2013-03-25 10:29 UTC #0 lore raw

PATCH 1/1 af_unix: dont send SCM_CREDENTIAL when dest socket is NULL

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 <[email protected]>

Signed-off-by Ding Tianhong <[email protected]>

---
   net/unix/af_unix.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

net/unix/af_unix.c +2 -2

index 51be64f..99189fd 100644
@@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
          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();
          }

Eric Dumazet

Acked-by on v1 1/1

2013-03-25 14:04 UTC #1 lore raw

Quoted 15 lines · dingtianhong

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 <[email protected]>
Signed-off-by Ding Tianhong <[email protected]>
---
   net/unix/af_unix.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
net/unix/af_unix.c +0 -0
index 51be64f..99189fd 100644

net/unix/af_unix.c static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, · line 1420

Quoted 10 lines

net/unix/af_unix.c +2 -2

@@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
          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();
          }

I am not sure why adding credentials if other->sk_socket is NULL could break an application ?

This was the case before commit introducing this code.

Anyway patch looks fine

Acked-by Eric Dumazet <[email protected]>

David Miller

maintainer

2013-03-25 17:12 UTC #2 lore raw

From: Eric Dumazet <[email protected]>
Date: Mon, 25 Mar 2013 07:04:29 -0700

Quoted 30 lines · dingtianhong

Quoted 15 lines
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 <[email protected]>
Signed-off-by Ding Tianhong <[email protected]>
---
   net/unix/af_unix.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
net/unix/af_unix.c +0 -0
index 51be64f..99189fd 100644
net/unix/af_unix.c static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, · line 1420
Quoted 10 lines
net/unix/af_unix.c +2 -2
@@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
          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();
          }
I am not sure why adding credentials if other->sk_socket is NULL could break an application ?

Quoted 5 lines

This was the case before commit introducing this code.
Anyway patch looks fine
Acked-by Eric Dumazet <[email protected]>

This patch is massively whitespace damaged, and therefore completely unusable.

Please fix this, and only resubmit this patch when you can successfully email the patch to yourself and apply it successfully.

dingtianhong

author

2013-03-26 03:09 UTC #3 lore raw

In reply to Eric Dumazet

Quoted 21 lines · Eric Dumazet

Quoted 20 lines · dingtianhong
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 <[email protected]>
Signed-off-by Ding Tianhong <[email protected]>
---
   net/unix/af_unix.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
net/unix/af_unix.c +0 -2
index 51be64f..99189fd 100644
@@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
          if (UNIXCB(skb).cred)
                  return;
          if (test_bit(SOCK_PASSCRED, &sock->flags) ||
-           !other->sk_socket ||
-           test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) {

Quoted 8 lines

net/unix/af_unix.c static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, · line 1420
Quoted 5 lines
net/unix/af_unix.c +2 -0
+           (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();
          }
I am not sure why adding credentials if other->sk_socket is NULL could break an application ?

The bugzilla has report the bug:https://lsbbugs.linuxfoundation.org/show_bug.cgi?id=3523

Quoted 2 lines

This was the case before commit introducing this code.

The commit 16e5726269(af_unix: dont send SCM_CREDENTIALS by default) may introducing the problem.

Quoted 10 lines

Anyway patch looks fine
Acked-by Eric Dumazet <[email protected]>

Eric Dumazet

2013-03-26 04:32 UTC #4 lore raw

Quoted 30 lines · dingtianhong

Quoted 21 lines · Eric Dumazet
Quoted 20 lines · dingtianhong
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 <[email protected]>
Signed-off-by Ding Tianhong <[email protected]>
---
   net/unix/af_unix.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
net/unix/af_unix.c +0 -2
index 51be64f..99189fd 100644
@@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
          if (UNIXCB(skb).cred)
                  return;
          if (test_bit(SOCK_PASSCRED, &sock->flags) ||
-           !other->sk_socket ||
-           test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) {
Quoted 8 lines
net/unix/af_unix.c static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, · line 1420
Quoted 5 lines
net/unix/af_unix.c +2 -0
+           (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();
          }
I am not sure why adding credentials if other->sk_socket is NULL could break an application ?

Quoted 2 lines

The bugzilla has report the bug:https://lsbbugs.linuxfoundation.org/show_bug.cgi?id=3523

Quoted 5 lines

Quoted 2 lines
This was the case before commit introducing this code.
The commit 16e5726269(af_unix: dont send SCM_CREDENTIALS by default) may introducing the problem.

So the problem is that two messages have different credentials, because other->sk_socket changed between first and second message.

and unix_stream_recvmsg() has the following check :

                if (check_creds) {
                        /* Never glue messages from different writers */
                        if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
                            (UNIXCB(skb).cred != siocb->scm->cred))
                                break;
                } else {
                        /* Copy credentials */
                        scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
                        check_creds = 1;
                }

In the case the receiver doesnt care at all (using recvfrom(), not recvmsg()), we probably should not even call scm_set_creds() and avoid extra refcounting.

dingtianhong

author

2013-03-26 11:36 UTC #5 lore raw

Quoted 58 lines · Eric Dumazet

Quoted 30 lines · dingtianhong
Quoted 21 lines · Eric Dumazet
Quoted 20 lines · dingtianhong
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 <[email protected]>
Signed-off-by Ding Tianhong <[email protected]>
---
   net/unix/af_unix.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
net/unix/af_unix.c +0 -2
index 51be64f..99189fd 100644
@@ -1413,8 +1413,8 @@ static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock,
          if (UNIXCB(skb).cred)
                  return;
          if (test_bit(SOCK_PASSCRED, &sock->flags) ||
-           !other->sk_socket ||
-           test_bit(SOCK_PASSCRED, &other->sk_socket->flags)) {
Quoted 8 lines
net/unix/af_unix.c static void maybe_add_creds(struct sk_buff *skb, const struct socket *sock, · line 1420
Quoted 5 lines
net/unix/af_unix.c +2 -0
+           (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();
          }
I am not sure why adding credentials if other->sk_socket is NULL could break an application ?
Quoted 2 lines
The bugzilla has report the bug:https://lsbbugs.linuxfoundation.org/show_bug.cgi?id=3523
OK
Quoted 5 lines
Quoted 2 lines
This was the case before commit introducing this code.
The commit 16e5726269(af_unix: dont send SCM_CREDENTIALS by default) may introducing the problem.
So the problem is that two messages have different credentials, because other->sk_socket changed between first and second message.
and unix_stream_recvmsg() has the following check :
                if (check_creds) {
                        /* Never glue messages from different writers */
                        if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
                            (UNIXCB(skb).cred != siocb->scm->cred))
                                break;
                } else {
                        /* Copy credentials */
                        scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
                        check_creds = 1;
                }

Quoted 3 lines

In the case the receiver doesnt care at all (using recvfrom(), not recvmsg()), we probably should not even call scm_set_creds() and avoid extra refcounting.

I think if not call scm_set_creds(), the credential would useles in recvmsg(). we could remove code:

		if (check_creds) {
                        /* Never glue messages from different writers */
                        if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
                            (UNIXCB(skb).cred != siocb->scm->cred))
                                break;
                } else {
                        /* Copy credentials */
                        scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
                        check_creds = 1;
                }

Quoted 5 lines

.

Eric Dumazet

2013-03-26 13:46 UTC #6 lore raw

Quoted 12 lines · dingtianhong

I think if not call scm_set_creds(), the credential would useles in recvmsg(). we could remove code:

		if (check_creds) {
                        /* Never glue messages from different writers */
                        if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
                            (UNIXCB(skb).cred != siocb->scm->cred))
                                break;
                } else {
                        /* Copy credentials */
                        scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
                        check_creds = 1;
                }

Are you paraphrasing me or saying something different ?

dingtianhong

author

2013-03-27 07:36 UTC #7 lore raw

PATCH 1/1 af_unix: dont send SCM_CREDENTIAL when dest socket is NULL

Quoted 15 lines · Eric Dumazet

Quoted 12 lines · dingtianhong

I think if not call scm_set_creds(), the credential would useles in recvmsg(). we could remove code:

		if (check_creds) {
                        /* Never glue messages from different writers */
                        if ((UNIXCB(skb).pid  != siocb->scm->pid) ||
                            (UNIXCB(skb).cred != siocb->scm->cred))
                                break;
                } else {
                        /* Copy credentials */
                        scm_set_cred(siocb->scm, UNIXCB(skb).pid, UNIXCB(skb).cred);
                        check_creds = 1;
                }

Quoted 2 lines

Are you paraphrasing me or saying something different ?

if not call scm_set_creds(), how get credentials for receiver and distinguish different writes, so I think scm_set_creds() is necessary here.

Quoted 3 lines