linux 2.9.10-rc1: Fix oops in unix_dgram_sendmsg when using SELinux and SOCK_SEQPACKET
Subsystems (3)
PATCH 1/1 linux 2.9.10-rc1: Fix oops in unix_dgram_sendmsg when using SELinux and SOCK_SEQPACKET
With CONFIG_SECURITY_NETWORK=y and CONFIG_SECURITY_SELINUX=y, using SOCK_SEQPACKET unix domain sockets causes an oops in the superfluous(?) call to security_unix_may_send in sock_dgram_sendmsg. This patch avoids making this call for SOCK_SEQPACKET sockets.
--- linux-2.6.10-rc1/net/unix/af_unix.c.orig 2004-11-13
21:04:53.000000000 +0000
+++ linux-2.6.10-rc1/net/unix/af_unix.c 2004-11-13 21:12:23.000000000 +0000
@@ -1354,9 +1354,11 @@ restart:
if (other->sk_shutdown & RCV_SHUTDOWN)
goto out_unlock;
- err = security_unix_may_send(sk->sk_socket, other->sk_socket);
- if (err)
- goto out_unlock;
+ if (sk->sk_type != SOCK_SEQPACKET) {
+ err = security_unix_may_send(sk->sk_socket, other->sk_socket);
+ if (err)
+ goto out_unlock;
+ }
if (unix_peer(other) != sk &&
(skb_queue_len(&other->sk_receive_queue) >
Quoted 4 lines · Ross Kendall Axe
With
CONFIG_SECURITY_NETWORK=yandCONFIG_SECURITY_SELINUX=y, usingSOCK_SEQPACKETunix domain sockets causes an oops in the superfluous(?) call tosecurity_unix_may_sendinsock_dgram_sendmsg. This patch avoids making this call forSOCK_SEQPACKETsockets.
I'd prefer to track down the actual issue in the SELinux code and correct it than just omit the security hook call entirely. Do you have the Oops output and a trivial test case? Thanks.
Signature · 2 lines
Stephen Smalley <[email protected]> National Security Agency
Quoted 9 lines · * Stephen Smalley ([email protected]) wrote:
Quoted 4 lines · Ross Kendall Axe
With
CONFIG_SECURITY_NETWORK=yandCONFIG_SECURITY_SELINUX=y, usingSOCK_SEQPACKETunix domain sockets causes an oops in the superfluous(?) call tosecurity_unix_may_sendinsock_dgram_sendmsg. This patch avoids making this call forSOCK_SEQPACKETsockets.I'd prefer to track down the actual issue in the SELinux code and correct it than just omit the security hook call entirely. Do you have the Oops output and a trivial test case? Thanks.
Well, there is one simple case that will trigger the Oops. Send a SEQPACKET to a connected but not yet accepted socket. In this case other->sk_socket is still NULL, and SELinux will deref the NULL pointer in selinux_socket_may_send() when geting other_isec. There is already a check in unix_stream_connect, which is all that's used for normal unix stream sockets. But the seqpacket socket then uses unix_dgram_sendmsg, so triggers the may_send check as well.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 4 lines · Chris Wright
Quoted 2 lines · * Stephen Smalley ([email protected]) wrote:
On Sun, 2004-11-14 at 13:13, Ross Kendall Axe wrote:
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
With
CONFIG_SECURITY_NETWORK=yandCONFIG_SECURITY_SELINUX=y, usingSOCK_SEQPACKETunix domain sockets causes an oops in the superfluous(?) call tosecurity_unix_may_sendinsock_dgram_sendmsg. This patch avoids making this call forSOCK_SEQPACKETsockets.I'd prefer to track down the actual issue in the SELinux code and correct it than just omit the security hook call entirely. Do you have the Oops output and a trivial test case? Thanks.
Oops at
http://www.rossaxe.pwp.blueyonder.co.uk/seqpacket-oops/seqpacket-oops.txt
and test case at
http://www.rossaxe.pwp.blueyonder.co.uk/seqpacket-oops/seqpacket-killer.tar.gz
Just run 'seqpacket-crashd & seqpacket-crash' a couple of times.
Quoted 11 lines
Well, there is one simple case that will trigger the Oops. Send a SEQPACKET to a connected but not yet accepted socket. In this case
other->sk_socketis still NULL, and SELinux will deref the NULL pointer inselinux_socket_may_send()when getingother_isec. There is already a check inunix_stream_connect, which is all that's used for normal unix stream sockets. But the seqpacket socket then usesunix_dgram_sendmsg, so triggers themay_sendcheck as well.thanks,
-chris
A possibility that hadn't occurred to me was using sendto to send packets without connecting. Is this supposed to work? If so, then my patch is indeed inappropriate. If not, then that needs fixing also.
Ross
Quoted 8 lines · Ross Kendall Axe
A possibility that hadn't occurred to me was using sendto to send packets without connecting. Is this supposed to work? If so, then my patch is indeed inappropriate. If not, then that needs fixing also.
Ross
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
This patch causes sendmsg on SOCK_SEQPACKET unix domain sockets to return EISCONN or ENOTSUPP as appropriate if the 'to' address is specified. It also causes recvmsg to return EINVAL on unconnected sockets. This behaviour is consistent with SOCK_STREAM sockets.
net/unix/af_unix.c renamed from net/unix/af_unix.c.orig +10 -0
@@ -1272,6 +1272,11 @@ static int unix_dgram_sendmsg(struct kio goto out;if (msg->msg_namelen) { +if (sk->sk_type ==SOCK_SEQPACKET ) { + err = sk->sk_state ==TCP_ESTABLISHED + ? -EISCONN : -EOPNOTSUPP ; + goto out; + } err =unix_mkname (sunaddr, msg->msg_namelen, &hash);if (err <0 ) goto out;
@@ -1531,6 +1536,11 @@ static int unix_dgram_recvmsg(struct kiostruct sk_buff *skb;int err; + err = -EINVAL ; +if (sk->sk_type ==SOCK_SEQPACKET && + sk->sk_state !=TCP_ESTABLISHED ) + goto out; + err = -EOPNOTSUPP ;if (flags&MSG_OOB ) goto out;
Quoted 8 lines · Ross Kendall Axe
Quoted 5 lines · Ross Kendall Axe
A possibility that hadn't occurred to me was using sendto to send packets without connecting. Is this supposed to work? If so, then my patch is indeed inappropriate. If not, then that needs fixing also.
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto() on a non connected socket should fail with ENOTCONN.
Quoted 2 lines
This patch causes sendmsg on
SOCK_SEQPACKETunix domain sockets to return EISCONN or ENOTSUPP as appropriate if the 'to' address is specified.
For sendto():
The address must be ignored on a connected mode socket (i.e. in this case).
According to the send(2) man page, we may return EISCONN if the address and addr length are not NULL and zero. I think that the man page is incorrect. Posix says that EISCONN means "A destination address was specified and the socket is already connected", not "A destination address was specified and the socket is connected mode". i.e. we should only return EISCONN if the socket is in a connected state.
I'm not sure if we should return any error at all if an address is supplied to sendto() on SOCK_SEQPACKET. We're only required to ignore it.
I would say that we should return an error as it is likely a progamming mistake in the application and we should let them know.
However, as mentioned above, I don't think EISCONN is appropriate in this case. EINVAL might be better.
Quoted 2 lines
It also causes recvmsg to return EINVAL on unconnected sockets. This behaviour is consistent with
SOCK_STREAMsockets.
This seems incorrect too, Posix says to use ENOTCONN.
There is a non SELinux-related bug lurking in this code. I got this oops when trying to kill a modified version of seqpacket-crash which keeps sending in a loop and uses sendto() and an address with SOCK_SEQPACKET.
------------[ cut here ]------------ kernel BUG at include/asm/spinlock.h:133! invalid operand: 0000 [#1] PREEMPT SMP Modules linked in: ipv6 binfmt_misc video ac e1000 3c59x CPU: 0 EIP: 0060:[<c03393b2>] Not tainted VLI EFLAGS: 00010282 (2.6.10-rc2)
EIP is at _spin_lock_bh+0x4b/0x55
eax: 0000000e ebx: f757b04c ecx: c038c60c edx: 00000292
esi: f757b04c edi: f73f096c ebp: c1bf8ed4 esp: c1bf8ec8
ds: 007b es: 007b ss: 0068
Process seqpacket-crash (pid: 4989, threadinfo=c1bf8000 task=f75fd530)
Stack: c034c39c c02c171e f757b02c c1bf8ee4 c02c171e f79448d4 f73f098c c1bf8f0c
c02be9d4 f73f0960 f757b02c 00000000 00000000 ffffffff f73f098c 00000000
dfff3b20 c1bf8f1c c02be96b 00000000 f79448d4 c1bf8f38 c0151b2c f73f098c
Call Trace:
[<c010336d>] show_stack+0x7a/0x90
[<c01034ee>] show_registers+0x152/0x1ca
[<c01036f5>] die+0x100/0x184
[<c0103b53>] do_invalid_op+0xd2/0xea
[<c010301b>] error_code+0x2b/0x30
[<c02c171e>] lock_sock+0x20/0x50
[<c02be9d4>] sock_fasync+0x45/0x147
[<c02be96b>] sock_close+0x19/0x3d
[<c0151b2c>] __fput+0x11d/0x15b
[<c015052a>] filp_close+0x42/0x74
[<c011a699>] put_files_struct+0x87/0xfc
[<c011b440>] do_exit+0x17b/0x48d
[<c011b7f9>] do_group_exit+0x32/0x9e
[<c0102525>] sysenter_past_esp+0x52/0x--------------------
i.e.:
static inline void _raw_spin_lock(spinlock_t *lock) {
#ifdef CONFIG_DEBUG_SPINLOCK
if (unlikely(lock->magic != SPINLOCK_MAGIC)) {
printk("eip: %p\n", __builtin_return_address(0));
BUG();
}#endif
- James
Signature · 2 lines
James Morris <[email protected]>
Quoted 1 line · James Morris
There is a non SELinux-related bug lurking in this code.
I also got this when trying to kill the server (which seems to run at 100% during exit after receving a message sent with sendto() + address):
Badness in sk_del_node_init at include/net/sock.h:343
[<c010339a>] dump_stack+0x17/0x19 [<c03193bc>] __unix_remove_socket+0x64/0x66 [<c03196e4>] unix_release_sock+0x2b/0x259 [<c02bdf07>] sock_release+0x7a/0xda [<c02be973>] sock_close+0x21/0x3d [<c0151b2c>] __fput+0x11d/0x15b [<c015052a>] filp_close+0x42/0x74 [<c0102525>] sysenter_past_esp+0x52/0x71
Which is:
static __inline__ int sk_del_node_init(struct sock *sk)
{
int rc = __sk_del_node_init(sk);
if (rc) {
/* paranoid for a while -acme */
WARN_ON(atomic_read(&sk->sk_refcnt) == 1); <-- here- James
Signature · 2 lines
James Morris <[email protected]>
Quoted 6 lines · * James Morris ([email protected]) wrote:
Quoted 1 line · James Morris
There is a non SELinux-related bug lurking in this code.
I also got this when trying to kill the server (which seems to run at 100% during exit after receving a message sent with
sendto()+ address):
I was seeing similar with my test (w/out SELinux), but got garbage for back trace.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 22 lines · James Morris
Quoted 8 lines · Ross Kendall Axe
Quoted 4 lines · Ross Kendall Axe
A possibility that hadn't occurred to me was using sendto to send packets without connecting. Is this supposed to work? If so, then my patch is indeed inappropriate. If not, then that needs fixing also.
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto()on a non connected socket should fail with ENOTCONN.Quoted 2 lines
This patch causes sendmsg on
SOCK_SEQPACKETunix domain sockets to return EISCONN or ENOTSUPP as appropriate if the 'to' address is specified.For
sendto():
Quoted 8 lines
The address must be ignored on a connected mode socket (i.e. in this case).
According to the send(2) man page, we may return EISCONN if the address
and addr length are not NULL and zero. I think that the man page is
incorrect. Posix says that EISCONN means "A destination address was
specified and the socket is already connected", not "A destination address
was specified and the socket is connected mode". i.e. we should only return EISCONN if the socket is in a connected state.
The man page then goes on to say "the error ENOTCONN is returned when the socket was not actually connected". Admittedly, this is not what my patch does; it returns ENOTSUPP, as do SOCK_STREAM sockets.
Quoted 3 lines
I'm not sure if we should return any error at all if an address is supplied to
sendto()onSOCK_SEQPACKET. We're only required to ignore it.I would say that we should return an error as it is likely a progamming mistake in the application and we should let them know.
This was, after all, the point of the patch. Well, that and closing the security hole opened by my earlier patch :-)
Quoted 3 lines
However, as mentioned above, I don't think EISCONN is appropriate in this case. EINVAL might be better.
I would say that ENOTSUPP all the time would be more sensible. However, my
choice of error codes was determined by the ones used by SOCK_STREAM.SOCK_SEQPACKET and SOCK_STREAM should use the same error codes, I would say. Further, they should use the codes specified by POSIX.
Quoted 6 lines
Quoted 3 lines
It also causes recvmsg to return EINVAL on unconnected sockets. This behaviour is consistent with
SOCK_STREAMsockets.This seems incorrect too, Posix says to use ENOTCONN.
That seems eminently sensible. Again, I was just cut-n'-pasting from SOCK_STREAM. If these error codes are wrong, then SOCK_STREAM also needs fixing.
Quoted 2 lines
There is a non SELinux-related bug lurking in this code.
IMHO, there never was an SELinux bug here. SELinux merely exposed an existing bug.
Quoted 4 lines
I got this oops
when trying to kill a modified version of seqpacket-crash which keeps sending in a loop and usessendto()and an address withSOCK_SEQPACKET.
I'm unable to reproduce that, or the bug you mention in your other message. Care to send us your code?
Quoted 2 lines
- James
I think that af_unix.c needs a bit of cleaning up. All of the functions are named as being stream vs dgram, even when the issue is connectionless vs connection-oriented. For example, unix_connectionless_connect would make a lot more sense than unix_dgram_connect. sendmsg and recvmsg are the worst since they require a mixture of SOCK_STREAM and SOCK_DGRAM semantics. It would be nice to rewite unix_dgram_recvmsg and unix_stream_recvmsg as four helper functions dealing with the connectionless, connection-oriented, datagram and stream operations and then have 3 wrapper functions (one for each socket type) calling the appropriate helpers. This is all strictly IMHO, of course.
Ross
Quoted 3 lines · Ross Kendall Axe
That seems eminently sensible. Again, I was just cut-n'-pasting from
SOCK_STREAM. If these error codes are wrong, thenSOCK_STREAMalso needs fixing.
An issue here though is that what impact will this have on existing applications?
Quoted 5 lines
Quoted 2 lines
There is a non SELinux-related bug lurking in this code.
IMHO, there never was an SELinux bug here. SELinux merely exposed an existing bug.
Looks like it, testing a fix now.
Quoted 2 lines
I'm unable to reproduce that, or the bug you mention in your other message. Care to send us your code?
See http://people.redhat.com/jmorris/net/seqpacket-killer-jm.tar.bz2
Quoted 3 lines
I think that
af_unix.cneeds a bit of cleaning up. All of the functions are named as being stream vs dgram, even when the issue is connectionless vs connection-oriented.
Agreed.
- James
Signature · 2 lines
James Morris <[email protected]>
Here's a fix for the SELinux related problem.
What's happening is that mixing stream and dgram ops for SEQPACKET is having some unfortunate side effects.
One of these is that there is a race between client sendmsg() and server accept(). The server child socket is attached via sock_graft() after the client has entered unix_dgram_sendmsg() and called
security_unix_may_send(sk->sk_socket, other->sk_socket);
other->sk_socket will thus be null, causing the oops in SELinux and any other LSM which tries to dereference the pointer.
The fix is a combination of some of Ross's ideas:
1) SOCK_SEQPACKET is connection oriented, and there no need to call security_unix_may_send() for each packet. security_unix_stream_connect() is sufficient.
2) Ensure that unix_dgram_sendmsg() fails for SOCK_SEQPACKET sockets which are not connected, otherwise someone could bypass LSM by sending on an unconnected socket.
Note that this only solves the problem for the LSM hook.
Patch below, please review.
The other issue discussed -- server goes into a hard loop (and/or various lock/refcount related bugs) when the client sends a message via sendto() with an address supplied -- needs to be resolved separately.
---
net/unix/af_unix.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
net/unix/af_unix.c +8 -3
@@ -1261,6 +1261,9 @@ static int unix_dgram_sendmsg(struct kiolong timeo;struct scm_cookie tmp_scm; +if (sk->sk_type ==SOCK_SEQPACKET && sk->sk_state !=TCP_ESTABLISHED ) +return -ENOTCONN ; +if (NULL == siocb->scm) siocb->scm = &tmp_scm; err =scm_send (sock, msg, siocb->scm);
@@ -1354,9 +1357,11 @@ restart:if (other->sk_shutdown &RCV_SHUTDOWN ) goto out_unlock; - err =security_unix_may_send (sk->sk_socket, other->sk_socket); -if (err) - goto out_unlock; +if (sk->sk_type !=SOCK_SEQPACKET ) { + err =security_unix_may_send (sk->sk_socket, other->sk_socket); +if (err) + goto out_unlock; + } if (unix_peer(other) != sk && (skb_queue_len(&other->sk_receive_queue) >
Quoted 2 lines · * James Morris ([email protected]) wrote:
What's happening is that mixing stream and dgram ops for SEQPACKET is having some unfortunate side effects.
Agreed.
Quoted 8 lines
One of these is that there is a race between client
sendmsg()and serveraccept(). The server child socket is attached viasock_graft()after the client has enteredunix_dgram_sendmsg()and calledsecurity_unix_may_send(sk->sk_socket, other->sk_socket);
other->sk_socketwill thus be null, causing the oops in SELinux and any other LSM which tries to dereference the pointer.
Yup. And it's not much of a race, the window is wide open. One malicious app simply has to do:
bind()listen()connect()send() <-- Oops
Quoted 5 lines
The fix is a combination of some of Ross's ideas:
1)
SOCK_SEQPACKETis connection oriented, and there no need to callsecurity_unix_may_send()for each packet.security_unix_stream_connect()is sufficient.
Why not make a unix_seq_sendmsg, which is a very small wrapper? e.g.
static int unix_seq_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
struct sock *sk = sock->sk;
if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED)
return -ENOTCONN;
if (msg->msg_name || msg->msg_namelen)
return -EINVAL;
return unix_dgram_sendmsg(kiocb, sock, msg, len);
}Also, I missed how MSG_EOR is honored.
Quoted 3 lines
2) Ensure that
unix_dgram_sendmsg()fails forSOCK_SEQPACKETsockets which are not connected, otherwise someone could bypass LSM by sending on an unconnected socket.
Agreed, not connected, it should fail IMHO.
Quoted 1 line
Note that this only solves the problem for the LSM hook.
Does the above stop the other issue? My laptop died, so I'm not able to test ATM.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 1 line · Chris Wright
Why not make a
unix_seq_sendmsg, which is a very small wrapper?
Good idea, patch forthcoming.
Quoted 2 lines
Does the above stop the other issue? My laptop died, so I'm not able to test ATM.
No, it seems to be caused when addrlen in sendto() is non-zero, causing unix_find_other() to be called instead of unix_peer_get(), which is screwing up reference counts.
As for MSG_EOR, apart from the generic socket code, nothing is being done. This would be a separate issue.
- James
Signature · 2 lines
James Morris <[email protected]>
Updated patch below (with Chris Wright's wrapper idea).
This now fixes both issues.
1) Don't call security_unix_may_send() hook during sendmsg() for SOCK_SEQPACKET, and ensure that sendmsg() can only be called on a connected socket so as not to bypass the security_unix_stream_connect() hook.
2) Return -EINVAL if sendto() is called on SOCK_SEQPACKET with an address supplied.
Please review and apply if ok.
---
net/unix/af_unix.c | 26 ++++++++++++++++++++++---- 1 files changed, 22 insertions(+), 4 deletions(-)
net/unix/af_unix.c +22 -4
@@ -466,6 +466,8 @@ static int unix_dgram_recvmsg(struct kio struct msghdr *, size_t, int);static int unix_dgram_connect (struct socket *,struct sockaddr *,int ,int ); +static int unix_seqpacket_sendmsg (struct kiocb *,struct socket *, +struct msghdr *,size_t );static struct proto_ops unix_stream_ops = { .family =PF_UNIX ,
@@ -524,7 +526,7 @@ static struct proto_ops unix_seqpacket_o .shutdown = unix_shutdown, .setsockopt = sock_no_setsockopt, .getsockopt = sock_no_getsockopt, - .sendmsg = unix_dgram_sendmsg, + .sendmsg = unix_seqpacket_sendmsg, .recvmsg = unix_dgram_recvmsg, .mmap = sock_no_mmap, .sendpage = sock_no_sendpage,
@@ -1354,9 +1356,11 @@ restart:if (other->sk_shutdown &RCV_SHUTDOWN ) goto out_unlock; - err =security_unix_may_send (sk->sk_socket, other->sk_socket); -if (err) - goto out_unlock; +if (sk->sk_type !=SOCK_SEQPACKET ) { + err =security_unix_may_send (sk->sk_socket, other->sk_socket); +if (err) + goto out_unlock; + } if (unix_peer(other) != sk && (skb_queue_len(&other->sk_receive_queue) >
@@ -1506,6 +1510,20 @@ out_err:return sent ? : err; } +static int unix_seqpacket_sendmsg (struct kiocb *kiocb,struct socket *sock, +struct msghdr *msg,size_t len) +{ +struct sock *sk = sock->sk; + +if (sk->sk_state !=TCP_ESTABLISHED ) +return -ENOTCONN ; + +if (msg->msg_name || msg->msg_namelen) +return -EINVAL ; + +return unix_dgram_sendmsg (kiocb, sock, msg, len); +} +static void unix_copy_addr (struct msghdr *msg,struct sock *sk) {struct unix_sock *u =unix_sk (sk);
Quoted 7 lines · James Morris
1) Don't call
security_unix_may_send()hook duringsendmsg()forSOCK_SEQPACKET, and ensure thatsendmsg()can only be called on a connected socket so as not to bypass thesecurity_unix_stream_connect()hook.2) Return
-EINVALifsendto()is called onSOCK_SEQPACKETwith an address supplied.
Consider shutdown(). A sendmsg into shutdown must return the pending ECONNRESET
first.
David S. Miller
maintainerQuoted 9 lines · On Thu, 18 Nov 2004 12:25:21 -0500 (EST) James Morris <[email protected]>…
Updated patch below (with Chris Wright's wrapper idea).
This now fixes both issues.
1) Don't call
security_unix_may_send()hook duringsendmsg()forSOCK_SEQPACKET, and ensure thatsendmsg()can only be called on a connected socket so as not to bypass thesecurity_unix_stream_connect()hook.
Quoted 7 lines
2) Return
-EINVALifsendto()is called onSOCK_SEQPACKETwith an address supplied.Please review and apply if ok.
Signed-off-by James Morris <[email protected]>
Looks good, applied thanks James.
Quoted 4 lines · * James Morris ([email protected]) wrote:
Quoted 1 line · Chris Wright
Why not make a
unix_seq_sendmsg, which is a very small wrapper?
Quoted 8 lines
Good idea, patch forthcoming.
Quoted 2 lines
Does the above stop the other issue? My laptop died, so I'm not able to test ATM.
No, it seems to be caused when addrlen in
sendto()is non-zero, causingunix_find_other()to be called instead ofunix_peer_get(), which is screwing up reference counts.
Right, but the snippet I posted guards against that I think. It forces unix_peer_get() in dgram_sendmsg.
Quoted 2 lines
As for
MSG_EOR, apart from the generic socket code, nothing is being done. This would be a separate issue.
Yup, just noting the bits that looked broken to me.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 2 lines · Chris Wright
Right, but the snippet I posted guards against that I think. It forces
unix_peer_get()indgram_sendmsg.
Correct. We could also add more code to dgram_sendmsg() to simply ignore any address passed in for SOCK_SEQPACKETs, but again, I think that is a programming error and I think we should signal that, especially as this is a new feature.
- James
Signature · 2 lines
James Morris <[email protected]>
Quoted 16 lines · Chris Wright
Why not make a
unix_seq_sendmsg, which is a very small wrapper? e.g.
static int unix_seq_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg,size_tlen){ struct sock *sk = sock->sk; if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED) return -ENOTCONN; if (msg->msg_name || msg->msg_namelen) return -EINVAL; return unix_dgram_sendmsg(kiocb, sock, msg, len); }
Quoted 1 line
-chris
Taking this idea further, couldn't we split unix_dgram_sendmsg into 2 functions, do_unix_dgram_sendmsg and do_unix_connectionless_sendmsg (and similarly for unix_stream_sendmsg), then all we'd need is:
<pseudocode>
static int do_unix_dgram_sendmsg(...);
static int do_unix_stream_sendmsg(...);
static int do_unix_connectionless_sendmsg(...);
static int do_unix_connectional_sendmsg(...);
static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
return do_unix_connectionless_sendmsg(kiocb, sock, msg, len,
do_unix_dgram_sendmsg);
}
static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
return do_unix_connectional_sendmsg(kiocb, sock, msg, len,
do_unix_stream_sendmsg);
}
static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
return do_unix_connectional_sendmsg(kiocb, sock, msg, len,
do_unix_dgram_sendmsg);
}</pseudocode>
What do we think?
Ross
Quoted 9 lines · * Ross Kendall Axe ([email protected]) wrote:
Taking this idea further, couldn't we split
unix_dgram_sendmsginto 2 functions,do_unix_dgram_sendmsganddo_unix_connectionless_sendmsg(and similarly forunix_stream_sendmsg), then all we'd need is:<pseudocode>
static intdo_unix_dgram_sendmsg(...);
static intdo_unix_stream_sendmsg(...);
static intdo_unix_connectionless_sendmsg(...);
static intdo_unix_connectional_sendmsg(...);
We could probably break it down to better functions and helpers, but I'm not sure that's quite the breakdown. That looks to me like an indirect way to pass a flag which is already encoded in the ops and sk_type. At anyrate, for 2.6.10 the changes should be small and obvious. Better refactoring should be left for 2.6.11.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 11 lines · Chris Wright
Quoted 7 lines · * Ross Kendall Axe ([email protected]) wrote:
Taking this idea further, couldn't we split
unix_dgram_sendmsginto 2 functions,do_unix_dgram_sendmsganddo_unix_connectionless_sendmsg(and similarly forunix_stream_sendmsg), then all we'd need is:<pseudocode>
static intdo_unix_dgram_sendmsg(...);
static intdo_unix_stream_sendmsg(...);
static intdo_unix_connectionless_sendmsg(...);
static intdo_unix_connectional_sendmsg(...);
Quoted 3 lines
We could probably break it down to better functions and helpers, but I'm not sure that's quite the breakdown. That looks to me like an indirect way to pass a flag which is already encoded in the ops and
sk_type.
The idea of that breakdown was to encode the semantics purely into the ops and mostly ignore sk_type. An alternative would be to create a couple of macros is_connectionless and is_stream and lump it all together in one big unix_sendmsg. Unfortunately, unix_sendmsg could end up a bit too large. IMHO, unix_{dgram,stream}_sendmsg are large as it is.
Quoted 2 lines
At anyrate, for 2.6.10 the changes should be small and obvious. Better refactoring should be left for 2.6.11.
Agreed. I had my eye on 2.6.11 anyway.
Quoted 3 lines
thanks,
-chris
Ross
Quoted 11 lines · Chris Wright
Quoted 7 lines · * Ross Kendall Axe ([email protected]) wrote:
Taking this idea further, couldn't we split
unix_dgram_sendmsginto 2 functions,do_unix_dgram_sendmsganddo_unix_connectionless_sendmsg(and similarly forunix_stream_sendmsg), then all we'd need is:<pseudocode>
static intdo_unix_dgram_sendmsg(...);
static intdo_unix_stream_sendmsg(...);
static intdo_unix_connectionless_sendmsg(...);
static intdo_unix_connectional_sendmsg(...);
Quoted 5 lines
We could probably break it down to better functions and helpers, but I'm not sure that's quite the breakdown. That looks to me like an indirect way to pass a flag which is already encoded in the ops and
sk_type. At anyrate, for 2.6.10 the changes should be small and obvious. Better refactoring should be left for 2.6.11.
Hey, go ahead, do the split and please, please use sk->sk_prot, that is the way to do the proper split and will allow us to nuke several pointers in struct sock (sk_slab, sk_owner for now) :-)
I have a friend doing this for X.25, will submit his patches as soon as we do some more testing and 2.6.10 is out.
- Arnaldo
Quoted 23 lines · Arnaldo Carvalho de Melo
Quoted 12 lines · Chris Wright
Quoted 7 lines · * Ross Kendall Axe ([email protected]) wrote:
Taking this idea further, couldn't we split
unix_dgram_sendmsginto 2 functions,do_unix_dgram_sendmsganddo_unix_connectionless_sendmsg(and similarly forunix_stream_sendmsg), then all we'd need is:<pseudocode>
static intdo_unix_dgram_sendmsg(...);
static intdo_unix_stream_sendmsg(...);
static intdo_unix_connectionless_sendmsg(...);
static intdo_unix_connectional_sendmsg(...);Quoted 5 lines
We could probably break it down to better functions and helpers, but I'm not sure that's quite the breakdown. That looks to me like an indirect way to pass a flag which is already encoded in the ops and
sk_type. At anyrate, for 2.6.10 the changes should be small and obvious. Better refactoring should be left for 2.6.11.
Quoted 6 lines
Hey, go ahead, do the split and please, please use
sk->sk_prot, that is the way to do the proper split and will allow us to nuke several pointers in struct sock (sk_slab,sk_ownerfor now) :-)I have a friend doing this for X.25, will submit his patches as soon as we do some more testing and 2.6.10 is out.
Ah, this is the way the inet transport protos have been working for years, and I've been factoring out the struct proto_ops methods from TCP into the networking core, look at net/core/stream.c and the sock_common_ prefixed functions in net/core/sock.c.
- Arnaldo
Quoted 3 lines · James Morris
2) Ensure that
unix_dgram_sendmsg()fails forSOCK_SEQPACKETsockets which are not connected, otherwise someone could bypass LSM by sending on an unconnected socket.
What about half closed and other connected states ? This patch seems inadequate for things like X.25
Quoted 7 lines · Alan Cox
Quoted 3 lines · James Morris
2) Ensure that
unix_dgram_sendmsg()fails forSOCK_SEQPACKETsockets which are not connected, otherwise someone could bypass LSM by sending on an unconnected socket.What about half closed and other connected states ? This patch seems inadequate for things like X.25
The patch only affects the Unix code and does not change existing semantics for other connected states.
One thing that looks broken (unrelated to the patch I posted) is that unix_dgram_sendmsg() already does not check sk->sk_shutdown & SEND_SHUTDOWN for SOCK_SEQPACKET.
- James
Signature · 2 lines
James Morris <[email protected]>
Quoted 3 lines · James Morris
One thing that looks broken (unrelated to the patch I posted) is that
unix_dgram_sendmsg()already does not checksk->sk_shutdown&SEND_SHUTDOWNforSOCK_SEQPACKET.
Looks like a real bug yes.
As to the other stuff I think the only change needed is to check the queued asynchronous error and report that before going on to the connected test
Quoted 3 lines · Alan Cox
As to the other stuff I think the only change needed is to check the queued asynchronous error and report that before going on to the connected test
How about this?
(Also now ignores any supplied address per
http://www.opengroup.org/onlinepubs/009695399/functions/sendto.html)
---
net/unix/af_unix.c +27 -4
@@ -466,6 +466,8 @@ static int unix_dgram_recvmsg(struct kio struct msghdr *, size_t, int);static int unix_dgram_connect (struct socket *,struct sockaddr *,int ,int ); +static int unix_seqpacket_sendmsg (struct kiocb *,struct socket *, +struct msghdr *,size_t );static struct proto_ops unix_stream_ops = { .family =PF_UNIX ,
@@ -524,7 +526,7 @@ static struct proto_ops unix_seqpacket_o .shutdown = unix_shutdown, .setsockopt = sock_no_setsockopt, .getsockopt = sock_no_getsockopt, - .sendmsg = unix_dgram_sendmsg, + .sendmsg = unix_seqpacket_sendmsg, .recvmsg = unix_dgram_recvmsg, .mmap = sock_no_mmap, .sendpage = sock_no_sendpage,
@@ -1354,9 +1356,11 @@ restart:if (other->sk_shutdown &RCV_SHUTDOWN ) goto out_unlock; - err =security_unix_may_send (sk->sk_socket, other->sk_socket); -if (err) - goto out_unlock; +if (sk->sk_type !=SOCK_SEQPACKET ) { + err =security_unix_may_send (sk->sk_socket, other->sk_socket); +if (err) + goto out_unlock; + } if (unix_peer(other) != sk && (skb_queue_len(&other->sk_receive_queue) >
@@ -1506,6 +1510,25 @@ out_err:return sent ? : err; } +static int unix_seqpacket_sendmsg (struct kiocb *kiocb,struct socket *sock, +struct msghdr *msg,size_t len) +{ +int err; +struct sock *sk = sock->sk; + + err =sock_error (sk); +if (err) +return err; + +if (sk->sk_state !=TCP_ESTABLISHED ) +return -ENOTCONN ; + +if (msg->msg_namelen) + msg->msg_namelen =0 ; + +return unix_dgram_sendmsg (kiocb, sock, msg, len); +} +static void unix_copy_addr (struct msghdr *msg,struct sock *sk) {struct unix_sock *u =unix_sk (sk);
Quoted 7 lines · * James Morris ([email protected]) wrote:
Quoted 3 lines · Alan Cox
As to the other stuff I think the only change needed is to check the queued asynchronous error and report that before going on to the connected test
How about this?
The other patch is already committed, so relative diff would be needed.
Quoted 2 lines
(Also now ignores any supplied address per
http://www.opengroup.org/onlinepubs/009695399/functions/sendto.html)
Nitpick, but I missed where it said ignore the address. And it seems counter intuitive to provide address, only to have it ignored and delivered elsewhere.
thanks,
-chris
Quoted 6 lines · Chris Wright
Quoted 2 lines
(Also now ignores any supplied address per
http://www.opengroup.org/onlinepubs/009695399/functions/sendto.html)Nitpick, but I missed where it said ignore the address. And it seems counter intuitive to provide address, only to have it ignored and delivered elsewhere.
For sendto():
"If the socket is connection-mode, dest_addr shall be ignored."
For sendmsg():
"If the socket is connection-mode, the destination address in msghdr
shall be ignored."
I agree that it's counter intuitive (and surely an application bug), but some feedback I've had suggests we follow the spec.
- James
Signature · 2 lines
James Morris <[email protected]>
Quoted 2 lines · * James Morris ([email protected]) wrote:
For
sendto():
"If the socket is connection-mode,dest_addrshall be ignored."
Yup, I read right past it, thanks. Guess the standards have spoken ;-)
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Quoted 7 lines · James Morris
Quoted 3 lines · Alan Cox
As to the other stuff I think the only change needed is to check the queued asynchronous error and report that before going on to the connected test
How about this?
Looks right to me, the ECONNRESET is no longer being lost.
Quoted 1 line · Alan Cox
Looks right to me, the ECONNRESET is no longer being lost.
Ok, here is a relative patch for Dave.
Please apply.
---
net/unix/af_unix.c +7 -2
@@ -1513,13 +1513,18 @@ out_err:static int unix_seqpacket_sendmsg (struct kiocb *kiocb,struct socket *sock,struct msghdr *msg,size_t len) { +int err;struct sock *sk = sock->sk; + err =sock_error (sk); +if (err) +return err; +if (sk->sk_state !=TCP_ESTABLISHED )return -ENOTCONN ; -if (msg->msg_name || msg->msg_namelen) -return -EINVAL ; +if (msg->msg_namelen) + msg->msg_namelen =0 ;return unix_dgram_sendmsg (kiocb, sock, msg, len); }
Quoted 9 lines · On Fri, 19 Nov 2004 11:24:03 -0500 (EST) James Morris <[email protected]>…
Quoted 1 line · Alan Cox
Looks right to me, the ECONNRESET is no longer being lost.
Ok, here is a relative patch for Dave.
Please apply.
Signed-off-by James Morris <[email protected]>
Applied, thanks for fixing this up.
Quoted 3 lines · James Morris
Quoted 1 line
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto()on a non connected socket should fail with ENOTCONN.
Not entirely true at all. A network protocol can implement lazy binding and
do implicit binding on the sendto. Other protocols might not actually have
a receiving component so have no bind() functionality at all.
Quoted 6 lines
According to the send(2) man page, we may return EISCONN if the address and addr length are not NULL and zero. I think that the man page is incorrect. Posix says that EISCONN means "A destination address was specified and the socket is already connected", not "A destination address was specified and the socket is connected mode". i.e. we should only return EISCONN if the socket is in a connected state.
POSIX 1003.1g draft 6.4 permits a user to pass a "null" address for various things. Indeed some systems implement send() as sendto() with a NULL, 0 address component and some user space does likewise. It also has a lot to say on the other cases although I don't think it ever fully got past draft state.
You also want to look at TCP/IP illustrated to see some of the assumptions handed down from on high by BSD and which should not be broken.
Quoted 9 lines · Alan Cox
Quoted 3 lines · James Morris
Quoted 1 line
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto()on a non connected socket should fail with ENOTCONN.Not entirely true at all. A network protocol can implement lazy binding and do implicit binding on the sendto. Other protocols might not actually have a receiving component so have no
bind()functionality at all.
I got this from the Linux man page for sendto():
If sendto is used on a connection-mode (SOCK_STREAM, SOCK_SEQPACKET) socket, the parameters to and tolen are ignored (and the error EISCONN may be returned when they are not NULL and 0), and the error ENOTCONN is returned when the socket was not actually connected.
And Posix 1003.1 offers the following error code for sendto():
The sendto() function shall fail if:
...
[ENOTCONN]
The socket is connection-mode but is not connected.
(I'm not saying you're wrong).
Quoted 5 lines
POSIX 1003.1g draft 6.4 permits a user to pass a "null" address for various things. Indeed some systems implement
send()assendto()with a NULL, 0 address component and some user space does likewise. It also has a lot to say on the other cases although I don't think it ever fully got past draft state.
sendto() with a NULL address will still work fine.
- James
Signature · 2 lines
James Morris <[email protected]>
Quoted 10 lines · * Alan Cox ([email protected]) wrote:
Quoted 3 lines · James Morris
Quoted 1 line
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto()on a non connected socket should fail with ENOTCONN.Not entirely true at all. A network protocol can implement lazy binding and
do implicit binding on the sendto. Other protocols might not actually have
a receiving component so have nobind()functionality at all.
Just to be clear, this fix is not at socket layer, but specific to UNIX domain socket protocol layer.
thanks,
-chris
Signature · 1 line
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net