archive mirror
 help / color / mirror / Atom feed
* [PATCH v0] strparser: remove any offset before parsing messages
@ 2018-08-09 22:40 Dominique Martinet
  2018-08-21 12:51 ` [PATCH] " Dominique Martinet
  0 siblings, 1 reply; 12+ messages in thread
From: Dominique Martinet @ 2018-08-09 22:40 UTC (permalink / raw)
  To: Doron Roberts-Kedes, Tom Herbert, Dave Watson
  Cc: Dominique Martinet, David S. Miller, netdev, linux-kernel

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 the skb before parsing ensures that the offset
will be 0 everywhere in practice unless the user modifies it themselves
like tls, as a workaround for the other two protocols.

This fixes a bug whilch can be exhibited by implementing a simpe 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

Signed-off-by: Dominique Martinet <>

Discussions on the bug along with a (bad) reproducer can be found here:
(now the problem is better understood though it's much simpler to send
two messages at once than to spam and wait for tcp aggregation to do it)

Two notes:
 - I've marked this patch v0 as we could move the pskb_pull() up to
where strp.offset is set, and just always leave it at 0 in the strparser
This will let applications that are fine dealing with a non-zero offset
deal with it as they seem fit (tls writes into the offset and full_len
fields behind the back of the stream parser), while still being safe for

 - Even with that modification I'm not totally happy with
single-handedly eating the offset for strparser users which could handle
it, but I'm not really familiar with the cost this really has in
A better fix would be to handle the offset properly in the callbacks,
but frankly at least for kcm I don't see how (maybe because I'm not
familiar with how bpf programs work)

Another idea I had would be to write flags when registering the protocol
e.g. strp->cb.flags & STRP_CAN_PARSE_WITH_OFFSET or something like that,
but without an idea of the cost of that pull I don't know if it's worth

Anyway, comments welcome.

 net/strparser/strparser.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 625acb27efcc..d7a3b81c3481 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) {
@@ -249,8 +259,7 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
 				strp_parser_err(strp, -EMSGSIZE, desc);
-			} 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)

^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-08-23  1:05 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-09 22:40 [PATCH v0] strparser: remove any offset before parsing messages Dominique Martinet
2018-08-21 12:51 ` [PATCH] " Dominique Martinet
2018-08-21 14:53   ` 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

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).