[PATCH] llc: SOCK_DGRAM interface fixes

[PATCH] llc: SOCK_DGRAM interface fixes

[Stephen Hemminger]

The datagram interface of LLC is broken in a couple of ways.
These were discovered when trying to use it to build an out-of-kernel
version of STP.

First it didn't pass the source address of the received packet
in recvfrom(). It needs to copy the source address of received LLC packets
into the socket control block. At the same time fix a security issue
because there was uninitialized data leakage. Every recvfrom call
was just copying out old data.

Second, LLC should not merge multiple packets in one receive call
on datagram sockets. LLC should preserve packet boundaries on
SOCK_DGRAM. 

This fix goes against the old historical comments about UNIX98 semantics
but without this fix SOCK_DGRAM is broken and useless. So either ANK's
interpretation was incorect or UNIX98 standard was wrong.


Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
---
 net/llc/af_llc.c  |   20 ++++++++------------
 net/llc/llc_sap.c |    4 ++--
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/net/llc/af_llc.c b/net/llc/af_llc.c
index d6cfe84..2652ead 100644
--- a/net/llc/af_llc.c
+++ b/net/llc/af_llc.c
@@ -784,24 +784,20 @@ static int llc_ui_recvmsg(struct kiocb *
 		copied += used;
 		len -= used;
 
-		if (used + offset < skb->len)
-			continue;
-
 		if (!(flags & MSG_PEEK)) {
 			sk_eat_skb(sk, skb, 0);
 			*seq = 0;
 		}
+
+		/* For non stream protcols we get one packet per recvmsg call */
+		if (sk->sk_type != SOCK_STREAM)
+			goto copy_uaddr;
+
+		/* Partial read */
+		if (used + offset < skb->len)
+			continue;
 	} while (len > 0);
 
-	/* 
-	 * According to UNIX98, msg_name/msg_namelen are ignored
-	 * on connected socket. -ANK
-	 * But... af_llc still doesn't have separate sets of methods for
-	 * SOCK_DGRAM and SOCK_STREAM :-( So we have to do this test, will
-	 * eventually fix this tho :-) -acme
-	 */
-	if (sk->sk_type == SOCK_DGRAM)
-		goto copy_uaddr;
 out:
 	release_sock(sk);
 	return copied;
diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
index 20c4eb5..42eb0c3 100644
--- a/net/llc/llc_sap.c
+++ b/net/llc/llc_sap.c
@@ -51,10 +51,10 @@ void llc_save_primitive(struct sock *sk,
 {
 	struct sockaddr_llc *addr;
 
-	if (skb->sk->sk_type == SOCK_STREAM) /* See UNIX98 */
-		return;
        /* save primitive for use by the user. */
 	addr		  = llc_ui_skb_cb(skb);
+
+	memset(addr, 0, sizeof(*addr));
 	addr->sllc_family = sk->sk_family;
 	addr->sllc_arphrd = skb->dev->type;
 	addr->sllc_test   = prim == LLC_TEST_PRIM;
-- 
1.4.0

Re: [PATCH] llc: SOCK_DGRAM interface fixes

[Arnaldo Carvalho de Melo]

Stephen Hemminger wrote:
> The datagram interface of LLC is broken in a couple of ways.

I don't doubt it, most of the testing was done on the SOCK_STREAM code,
Jay Schullist worked a bit on the dgram side of things but that was long
ago.

The plan is to use sk->sk_prot-> in the same way as the INET transport
protocols, to clearly separate STREAM from DGRAM code, etc. Its yet
another thing on my longish backlog, sigh.

Dave, please apply.

Acked-by: Arnaldo Carvalho de Melo <acme@mandriva.com>

- Arnaldo

Re: [PATCH] llc: SOCK_DGRAM interface fixes

[David Miller]

From: Arnaldo Carvalho de Melo <acme@mandriva.com>
Date: Thu, 03 Aug 2006 13:38:51 -0300

> Dave, please apply.
> 
> Acked-by: Arnaldo Carvalho de Melo <acme@mandriva.com>

Applied, thanks everyone.

Re: [stable] [PATCH] llc: SOCK_DGRAM interface fixes

[Greg KH]

On Thu, Aug 03, 2006 at 09:21:17AM -0700, Stephen Hemminger wrote:
> The datagram interface of LLC is broken in a couple of ways.
> These were discovered when trying to use it to build an out-of-kernel
> version of STP.
> 
> First it didn't pass the source address of the received packet
> in recvfrom(). It needs to copy the source address of received LLC packets
> into the socket control block. At the same time fix a security issue
> because there was uninitialized data leakage. Every recvfrom call
> was just copying out old data.
> 
> Second, LLC should not merge multiple packets in one receive call
> on datagram sockets. LLC should preserve packet boundaries on
> SOCK_DGRAM. 
> 
> This fix goes against the old historical comments about UNIX98 semantics
> but without this fix SOCK_DGRAM is broken and useless. So either ANK's
> interpretation was incorect or UNIX98 standard was wrong.

This doesn't apply at all to the current -stable tree, sorry.

greg k-h

Re: [PATCH] llc: SOCK_DGRAM interface fixes

[Alexey Kuznetsov]

Hello!

> This fix goes against the old historical comments about UNIX98 semantics
> but without this fix SOCK_DGRAM is broken and useless. So either ANK's
> interpretation was incorect or UNIX98 standard was wrong.

Just found this reference to me. :-)

The comment migrated from tcp.c. It is only about connected SOCK_STREAM
sockets, I do not see how it can make SOCK_DGRAM broken or useless.

That UNIX98 statement allowed to avoid expensive callback to protocol
specific setup of address in tcp_recvmsg().

Alexey

Re: [PATCH] llc: SOCK_DGRAM interface fixes

[Stephen Hemminger]

On Tue, 8 Aug 2006 20:36:18 +0400
Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote:

> Hello!
> 
> > This fix goes against the old historical comments about UNIX98 semantics
> > but without this fix SOCK_DGRAM is broken and useless. So either ANK's
> > interpretation was incorect or UNIX98 standard was wrong.
> 
> Just found this reference to me. :-)
> 
> The comment migrated from tcp.c. It is only about connected SOCK_STREAM
> sockets, I do not see how it can make SOCK_DGRAM broken or useless.

The code was in the place where the source information was being copied
from the data portion to the cb portion of the skb. The original code
did the save only for SOCK_STREAM.
The cb portion is needed later to produce the address portion of the
receive handling which is critical with SOCK_DGRAM in LLC.

> That UNIX98 statement allowed to avoid expensive callback to protocol
> specific setup of address in tcp_recvmsg().

So the comment made sense for TCP but not LLC.


-- 

Re: [PATCH] llc: SOCK_DGRAM interface fixes

[Arnaldo Carvalho de Melo]

Em Tue, Aug 08, 2006 at 08:36:18PM +0400, Alexey Kuznetsov escreveu:
> Hello!
> 
> > This fix goes against the old historical comments about UNIX98 semantics
> > but without this fix SOCK_DGRAM is broken and useless. So either ANK's
> > interpretation was incorect or UNIX98 standard was wrong.
> 
> Just found this reference to me. :-)
> 
> The comment migrated from tcp.c. It is only about connected SOCK_STREAM
> sockets, I do not see how it can make SOCK_DGRAM broken or useless.

Its just that the work to get the AF_LLC code closer to the AF_INET
structure was suspended before the SOCK_STREAM code was separated from the
SOCK_DGRAM one, moving PF_LLC to sk->sk_prot-> land, so the comment is
indeed about SOCK_STREAM part, not DGRAM.

- Arnaldo