netdev - [PATCH] strparser: remove any offset before parsing messages

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]

Message-Id: <1534855906-22870-1-git-send-email-asmadeus@codewreck.org>
Date:   Tue, 21 Aug 2018 14:51:46 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Doron Roberts-Kedes <doronrk@...com>,
        Tom Herbert <tom@...ntonium.net>,
        Dave Watson <davejwatson@...com>
Cc:     Dominique Martinet <asmadeus@...ewreck.org>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: [PATCH] strparser: remove any offset before parsing messages

Offset is not well handled by strparser users right now.

Out of the current strparser users, we have:
 - tls, that handles offset properly in parse and rcv callbacks
 - kcm, that handles offset in rcv but not in parse
 - bpf sockmap, that does not seem to handle offset anywhere

Calling pskb_pull() on new skb ensures that the offset will be 0
everywhere in practice, and in particular for the parse function,
unless the user modifies it themselves like tls does.

This fixes a bug which can be exhibited by implementing a simple kcm
parser that looks for the packet size in the first word of the packet,
and sending two such packets in a single write() call on the other side:
the second message will be cut at the length of the first message.
Since this is a stream protocol, all the following messages will also
be corrupt since it will start looking for the next offset at a wrong
position.

Signed-off-by: Dominique Martinet <asmadeus@...ewreck.org>
---

I haven't had any comment on v0, so here is what I had planned as
refactoring anyawy, but I'd *really* like some opinion on this as a
whole...

Also note that compiling bpf programs with libbcc is currently broken in
linus' master, see the fix thread here:
https://lkml.kernel.org/r/1534834088-15835-1-git-send-email-yamada.masahiro@socionext.com
You can likely just revert cafa0010cd51 ("Raise the minimum required gcc
version to 4.6") localy meanwhile, or base the patch off 4.18.

 net/strparser/strparser.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index da1a676860ca..d7fb30b1bcfc 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -201,7 +201,17 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
 			strp->skb_nextp = NULL;
 			stm = _strp_msg(head);
 			memset(stm, 0, sizeof(*stm));
-			stm->strp.offset = orig_offset + eaten;
+
+			/* Can only parse if there is no offset */
+			if (unlikely(orig_offset + eaten)) {
+				if (!pskb_pull(skb, orig_offset + eaten)) {
+					STRP_STATS_INCR(strp->stats.mem_fail);
+					strp_parser_err(strp, -ENOMEM, desc);
+					break;
+				}
+				orig_len -= eaten;
+				orig_offset = eaten = 0;
+			}
 		} else {
 			/* Unclone if we are appending to an skb that we
 			 * already share a frag_list with.
@@ -253,8 +263,7 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
 				STRP_STATS_INCR(strp->stats.msg_too_big);
 				strp_parser_err(strp, -EMSGSIZE, desc);
 				break;
-			} else if (len <= (ssize_t)head->len -
-					  skb->len - stm->strp.offset) {
+			} else if (len <= (ssize_t)head->len - skb->len) {
 				/* Length must be into new skb (and also
 				 * greater than zero)
 				 */
-- 
2.17.1