Re: [PATCH] linux 2.9.10-rc1: Fix oops in unix_dgram_sendmsg when using SELinux and SOCK_SEQPACKET

[Ross Kendall Axe <ross.axe@blueyonder.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 256 bytes --]