netdev - Re: KCM - recvmsg() mangles packets?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 4 Aug 2018 03:41:32 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Tom Herbert <tom@...ntonium.net>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: KCM - recvmsg() mangles packets?

Tom Herbert wrote on Fri, Aug 03, 2018:
> On Fri, Aug 3, 2018 at 4:20 PM, Dominique Martinet
> <asmadeus@...ewreck.org> wrote:
> > Tom Herbert wrote on Fri, Aug 03, 2018:
> >> struct my_proto {
> >>    struct _hdr {
> >>        uint32_t len;
> >>     } hdr;
> >>     char data[32];
> >> } __attribute__((packed));
> >>
> >> // use htons to use LE header size, since load_half does a first convertion
> >> // from network byte order
> >> const char *bpf_prog_string = " \
> >> ssize_t bpf_prog1(struct __sk_buff *skb) \
> >> { \
> >>     return bpf_htons(load_half(skb, 0)) + 4; \
> >> }";
> >
> > (Just to make sure I did fix it to htonl(load_word()) and I can confirm
> > there is no difference)
> 
> You also need to htonl for
> 
> my_msg.hdr.len = (i++ * 1312739ULL) % 31 + 1;

Thanks, but this looks correct to me - I was writing the header in
little endian order here and doing the double-swap dance in the bpf prog
because the protocol I was considering making a KCM implementation for
uses that.

Just to make sure, I rewrote it using network byte order e.g. these
three points and this makes no difference:
---8<----------------------
diff --git a/kcm.c b/kcm.c
index cb48df1..d437226 100644
--- a/kcm.c
+++ b/kcm.c
@@ -36,7 +36,7 @@ struct my_proto {
 const char *bpf_prog_string = "				\
 ssize_t bpf_prog1(struct __sk_buff *skb)		\
 {							\
-	return bpf_htons(load_half(skb, 0)) + 4;	\
+	return load_word(skb, 0) + 4;			\
 }";
 
 int servsock_init(int port)
@@ -110,13 +110,15 @@ void client(int port)
 
 	int i = 1;
 	while(1) {
-		my_msg.hdr.len = (i++ * 1312739ULL) % 31 + 1;
-		for (int j = 0; j < my_msg.hdr.len; ) {
-			j += snprintf(my_msg.data + j, my_msg.hdr.len - j, "%i", i - 1);
+		int len = (i++ * 1312739ULL) % 31 + 1;
+		my_msg.hdr.len = htonl(len);
+		for (int j = 0; j < len; ) {
+			j += snprintf(my_msg.data + j, len - j,
+				      "%i", i - 1);
 		}
-		my_msg.data[my_msg.hdr.len-1] = '\0';
-		//printf("%d: writing %d\n", i-1, my_msg.hdr.len);
-		len = write(s, &my_msg, sizeof(my_msg.hdr) + my_msg.hdr.len);
+		my_msg.data[len-1] = '\0';
+		//printf("%d: writing %d\n", i-1, len);
+		len = write(s, &my_msg, sizeof(my_msg.hdr) + len);
 		if (error == -1)
 			err(EXIT_FAILURE, "write");
 		//usleep(10000);
@@ -171,9 +173,10 @@ void process(int kcmfd)
 		len = recvmsg(kcmfd, &msg, 0);
 		if (len == -1)
 			err(EXIT_FAILURE, "recvmsg");
-		if (len != my_msg.hdr.len + 4) {
+		if (len != ntohl(my_msg.hdr.len) + 4) {
 			printf("Got %d, expected %d on %dth message: %s; flags:
			%x\n",
-			       len - 4, my_msg.hdr.len, i, my_msg.data, msg.msg_flags);
+			       len - 4, ntohl(my_msg.hdr.len), i,
+			       my_msg.data, msg.msg_flags);
 			exit(1);
 		}
 		i++;
----------------8<-----------

Frankly I do not believe this is a rule problem, as if the length
splitting was incorrect the program would not work at all, but just
uncommenting the usleep on the sender side makes this work.

Actually, now I'm looking closer to the timing, it looks specific to the
connection setup. This send loop works:
        int i = 1;
        while(i <= 1000) {
                int len = (i++ * 1312739ULL) % 31 + 1;
                my_msg.hdr.len = htonl(len);
                for (int j = 0; j < len; ) {
                        j += snprintf(my_msg.data + j, len - j,
                                      "%i", i - 1);
                }
                my_msg.data[len-1] = '\0';
                //printf("%d: writing %d\n", i-1, len);
                len = write(s, &my_msg, sizeof(my_msg.hdr) + len);
                if (error == -1)
                        err(EXIT_FAILURE, "write");
                if (i == 2)
                        usleep(1);
        }

But removing the usleep(1) after the first packet makes recvmsg()
"fail": it reads the content of the second packet with the size of the
first.


I assume that usleep gives the server time to finish setting up the kcm
socket, because it does accept(); ioctl(SIOCKCMATTACH); recvmsg(); but
the client does not wait to send packets so there could be some sort of
race with the attach and multiple packets?


FWIW I took the time to look at older kernel and this has been happening
ever since KCM got introduced in 4.6


Thanks,
-- 
Dominique