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:   Mon, 6 Aug 2018 01:39:21 +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?

Dominique Martinet wrote on Sun, Aug 05, 2018:
> It's getting late but I'll try adding a pskb_pull in there tomorrow, it
> would be better to make the bpf program start with an offset but I don't
> think that'll be easy to change...

I can confirm the following patch fixes the issue for me:
-----8<---------------------
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 625acb27efcc..348ff5945591 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -222,6 +222,16 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
 		if (!stm->strp.full_len) {
 			ssize_t len;

+			/* Can only parse if there is no offset */
+			if (unlikely(stm->strp.offset)) {
+				if (!pskb_pull(skb, stm->strp.offset)) {
+					STRP_STATS_INCR(strp->stats.mem_fail);
+					strp_parser_err(strp, -ENOMEM, desc);
+					break;
+				}
+				stm->strp.offset = 0;
+			}
+
 			len = (*strp->cb.parse_msg)(strp, head);

 			if (!len) {
----------------8<----------------------

Now, I was looking at other users of strparser (I see sockmap, kcm and
tls) and it looks like sockmap does not handle offsets either but tls
does by using skb_copy_bits -- they're copying the tls header to a
buffer on the stack.

kcm cannot do that because we do not know how much data the user expects
to read, and I'm not comfortable doing pskb_pull in the kcm callback
either, but the cost of this pull is probably non-negligible if some
user can make do without it...

On the other hand, I do not see how to make the bpf program handle an
offset in the skb as that offset is strparser-specific.

Maybe add a flag in the cb that specifies wether the callback allows
non-zero offset?

I'll let you see if you can reproduce this and will wait for advices on
how to solve this properly so we can work on a proper fix.

Thanks,
-- 
Dominique