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.
Signed-off-by: Ross Axe <[email protected]>
--- 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) >
On Sun, 2004-11-14 at 13:13, Ross Kendall Axe wrote:
> 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.
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.
--
Stephen Smalley <[email protected]>
National Security Agency
* Stephen Smalley ([email protected]) wrote:
> On Sun, 2004-11-14 at 13:13, Ross Kendall Axe wrote:
> > 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.
>
> 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
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Chris Wright wrote:
> * Stephen Smalley ([email protected]) wrote:
>
>>On Sun, 2004-11-14 at 13:13, Ross Kendall Axe wrote:
>>
>>>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.
>>
>>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.
>
> 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
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
Ross Kendall Axe wrote:
>
>
> 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.
signed-off-by: Ross Axe <[email protected]>
On Thu, 18 Nov 2004, Ross Kendall Axe wrote:
> Ross Kendall Axe wrote:
> >
> > 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.
> This patch causes sendmsg on SOCK_SEQPACKET unix 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.
> It also causes recvmsg to return EINVAL on unconnected sockets. This
> behaviour is consistent with SOCK_STREAM sockets.
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
--
James Morris
<[email protected]>
On Wed, 17 Nov 2004, James Morris wrote:
> 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
--
James Morris
<[email protected]>
* James Morris ([email protected]) wrote:
> On Wed, 17 Nov 2004, James Morris wrote:
>
> > 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
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
James Morris wrote:
> On Thu, 18 Nov 2004, Ross Kendall Axe wrote:
>
>
>>Ross Kendall Axe wrote:
>>
>>>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.
>
>
>>This patch causes sendmsg on SOCK_SEQPACKET unix 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.
>
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.
> 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.
This was, after all, the point of the patch. Well, that and closing the
security hole opened by my earlier patch :-)
>
> 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.
>
>>It also causes recvmsg to return EINVAL on unconnected sockets. This
>>behaviour is consistent with SOCK_STREAM sockets.
>
>
> 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.
>
> 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.
> 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.
>
I'm unable to reproduce that, or the bug you mention in your other
message. Care to send us your code?
>
> - 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
On Thu, 18 Nov 2004, Ross Kendall Axe wrote:
> 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.
An issue here though is that what impact will this have on existing
applications?
> >
> > 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.
> 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
> 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.
Agreed.
- James
--
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(-)
diff -purN -X dontdiff linux-2.6.10-rc2.o/net/unix/af_unix.c linux-2.6.10-rc2.w/net/unix/af_unix.c
--- linux-2.6.10-rc2.o/net/unix/af_unix.c 2004-11-15 13:18:56.000000000 -0500
+++ linux-2.6.10-rc2.w/net/unix/af_unix.c 2004-11-18 02:54:03.283777544 -0500
@@ -1261,6 +1261,9 @@ static int unix_dgram_sendmsg(struct kio
long 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) >
* James Morris ([email protected]) wrote:
> What's happening is that mixing stream and dgram ops for SEQPACKET is
> having some unfortunate side effects.
Agreed.
> 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.
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
> 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.
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.
> 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.
Agreed, not connected, it should fail IMHO.
> 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
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Thu, 18 Nov 2004, Chris Wright wrote:
> Why not make a unix_seq_sendmsg, which is a very small wrapper?
Good idea, patch forthcoming.
> 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
--
James Morris
<[email protected]>
* James Morris ([email protected]) wrote:
> On Thu, 18 Nov 2004, Chris Wright wrote:
>
> > Why not make a unix_seq_sendmsg, which is a very small wrapper?
>
> Good idea, patch forthcoming.
>
> > 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.
Right, but the snippet I posted guards against that I think. It forces
unix_peer_get() in dgram_sendmsg.
> 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
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Thu, 18 Nov 2004, Chris Wright wrote:
> Right, but the snippet I posted guards against that I think. It forces
> unix_peer_get() in dgram_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
--
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.
Signed-off-by: James Morris <[email protected]>
---
net/unix/af_unix.c | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
diff -purN -X dontdiff linux-2.6.10-rc2.o/net/unix/af_unix.c linux-2.6.10-rc2.w2/net/unix/af_unix.c
--- linux-2.6.10-rc2.o/net/unix/af_unix.c 2004-11-15 13:18:56.000000000 -0500
+++ linux-2.6.10-rc2.w2/net/unix/af_unix.c 2004-11-18 12:09:44.255462368 -0500
@@ -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);
On Iau, 2004-11-18 at 03:42, James Morris wrote:
> > 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.
> 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.
On Iau, 2004-11-18 at 08:27, James Morris wrote:
> 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.
What about half closed and other connected states ? This patch seems
inadequate for things like X.25
On Thu, 18 Nov 2004 12:25:21 -0500 (EST)
James Morris <[email protected]> wrote:
> 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.
>
>
> Signed-off-by: James Morris <[email protected]>
Looks good, applied thanks James.
On Iau, 2004-11-18 at 18:40, James Morris wrote:
> 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.
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
* Alan Cox ([email protected]) wrote:
> On Iau, 2004-11-18 at 03:42, James Morris wrote:
> > > 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.
Just to be clear, this fix is not at socket layer, but specific to UNIX
domain socket protocol layer.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Thu, 18 Nov 2004, Alan Cox wrote:
> On Iau, 2004-11-18 at 08:27, James Morris wrote:
> > 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.
>
> 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
--
James Morris
<[email protected]>
On Iau, 2004-11-18 at 17:25, James Morris wrote:
> 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.
Consider shutdown(). A sendmsg into shutdown must return the pending
ECONNRESET
first.
On Thu, 18 Nov 2004, Alan Cox wrote:
> On Iau, 2004-11-18 at 03:42, James Morris wrote:
> > > 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).
> 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.
sendto() with a NULL address will still work fine.
- James
--
James Morris
<[email protected]>
On Thu, 18 Nov 2004, Alan Cox wrote:
> 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)
---
Signed-off-by: James Morris <[email protected]>
diff -purN -X dontdiff linux-2.6.10-rc2.o/net/unix/af_unix.c linux-2.6.10-rc2.w3/net/unix/af_unix.c
--- linux-2.6.10-rc2.o/net/unix/af_unix.c 2004-11-15 13:18:56.000000000 -0500
+++ linux-2.6.10-rc2.w3/net/unix/af_unix.c 2004-11-18 21:54:12.650029672 -0500
@@ -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);
Chris Wright wrote:
>
> 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);
> }
>
>
> -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
* James Morris ([email protected]) wrote:
> On Thu, 18 Nov 2004, Alan Cox wrote:
>
> > 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.
> (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
On Thu, 18 Nov 2004, Chris Wright wrote:
> > (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
--
James Morris
<[email protected]>
* Ross Kendall Axe ([email protected]) wrote:
> 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(...);
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
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
* James Morris ([email protected]) wrote:
> For sendto():
> "If the socket is connection-mode, dest_addr shall be ignored."
Yup, I read right past it, thanks. Guess the standards have spoken ;-)
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
Chris Wright wrote:
> * Ross Kendall Axe ([email protected]) wrote:
>
>>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(...);
>
>
> 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.
> 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.
>
> thanks,
> -chris
Ross
On Gwe, 2004-11-19 at 03:12, James Morris wrote:
> On Thu, 18 Nov 2004, Alan Cox wrote:
>
> > 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.
Chris Wright wrote:
> * Ross Kendall Axe ([email protected]) wrote:
>
>>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(...);
>
>
> 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
Arnaldo Carvalho de Melo wrote:
>
>
> Chris Wright wrote:
>
>> * Ross Kendall Axe ([email protected]) wrote:
>>
>>> 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(...);
>>
>>
>>
>> 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.
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
On Fri, 19 Nov 2004, Alan Cox wrote:
> 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]>
---
diff -purN -X dontdiff linux-2.6.10-rc2.w2/net/unix/af_unix.c linux-2.6.10-rc2.w3/net/unix/af_unix.c
--- linux-2.6.10-rc2.w2/net/unix/af_unix.c 2004-11-18 12:09:44.000000000 -0500
+++ linux-2.6.10-rc2.w3/net/unix/af_unix.c 2004-11-18 21:54:12.000000000 -0500
@@ -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);
}
On Fri, 19 Nov 2004 11:24:03 -0500 (EST)
James Morris <[email protected]> wrote:
> On Fri, 19 Nov 2004, Alan Cox wrote:
>
> > 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.