From: Dominique Martinet <asmadeus@codewreck.org>
To: Doron Roberts-Kedes <doronrk@fb.com>,
Tom Herbert <tom@quantonium.net>,
Dave Watson <davejwatson@fb.com>
Cc: Dominique Martinet <asmadeus@codewreck.org>,
"David S. Miller" <davem@davemloft.net>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] strparser: remove any offset before parsing messages
Date: Tue, 21 Aug 2018 14:51:46 +0200 [thread overview]
Message-ID: <1534855906-22870-1-git-send-email-asmadeus@codewreck.org> (raw)
In-Reply-To: <1533854411-28184-1-git-send-email-asmadeus@codewreck.org>
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@codewreck.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
next prev parent reply other threads:[~2018-08-21 12:52 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-09 22:40 [PATCH v0] strparser: remove any offset before parsing messages Dominique Martinet
2018-08-21 12:51 ` Dominique Martinet [this message]
2018-08-21 14:53 ` [PATCH] " Doron Roberts-Kedes
2018-08-21 19:36 ` Dominique Martinet
2018-08-21 21:15 ` Doron Roberts-Kedes
2018-08-21 22:51 ` Dominique Martinet
2018-08-21 23:35 ` Doron Roberts-Kedes
2018-08-22 0:46 ` Dominique Martinet
2018-08-22 2:33 ` Doron Roberts-Kedes
2018-08-22 5:47 ` Dominique Martinet
2018-08-22 18:38 ` Dave Watson
2018-08-23 1:04 ` Dominique Martinet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1534855906-22870-1-git-send-email-asmadeus@codewreck.org \
--to=asmadeus@codewreck.org \
--cc=davejwatson@fb.com \
--cc=davem@davemloft.net \
--cc=doronrk@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=tom@quantonium.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).