| Message ID | 20181129055559.159228-15-sashal@kernel.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | None | expand |
Hello! On 11/29/2018 08:55 AM, Sasha Levin wrote: > From: Sven Eckelmann <sven@narfation.org> > > [ Upstream commit d7d8bbb40a5b1f682ee6589e212934f4c6b8ad60 ] > > The complete size ("total_size") of the fragmented packet is stored in the > fragment header and in the size of the fragment chain. When the fragments > are ready for merge, the skbuff's tail of the first fragment is expanded to > have enough room after the data pointer for at least total_size. This means > that it gets expanded by total_size - first_skb->len. > > But this is ignoring the fact that after expanding the buffer, the fragment > header is pulled by from this buffer. Assuming that the tailroom of the Pulled by what? > buffer was already 0, the buffer after the data pointer of the skbuff is > now only total_size - len(fragment_header) large. When the merge function > is then processing the remaining fragments, the code to copy the data over > to the merged skbuff will cause an skb_over_panic when it tries to actually > put enough data to fill the total_size bytes of the packet. > > The size of the skb_pull must therefore also be taken into account when the > buffer's tailroom is expanded. > > Fixes: 610bfc6bc99b ("batman-adv: Receive fragmented packets and merge") > Reported-by: Martin Weinelt <martin@darmstadt.freifunk.net> > Co-authored-by: Linus Lüssing <linus.luessing@c0d3.blue> > Signed-off-by: Sven Eckelmann <sven@narfation.org> > Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de> > Signed-off-by: Sasha Levin <sashal@kernel.org> [...] MBR, Sergei
On 11/29/2018 01:00 PM, Sergei Shtylyov wrote: >> From: Sven Eckelmann <sven@narfation.org> >> >> [ Upstream commit d7d8bbb40a5b1f682ee6589e212934f4c6b8ad60 ] >> >> The complete size ("total_size") of the fragmented packet is stored in the >> fragment header and in the size of the fragment chain. When the fragments >> are ready for merge, the skbuff's tail of the first fragment is expanded to >> have enough room after the data pointer for at least total_size. This means >> that it gets expanded by total_size - first_skb->len. >> >> But this is ignoring the fact that after expanding the buffer, the fragment >> header is pulled by from this buffer. Assuming that the tailroom of the > > Pulled by what? Oops, this was a -stable patch! Nevermind then. :-) >> buffer was already 0, the buffer after the data pointer of the skbuff is >> now only total_size - len(fragment_header) large. When the merge function >> is then processing the remaining fragments, the code to copy the data over >> to the merged skbuff will cause an skb_over_panic when it tries to actually >> put enough data to fill the total_size bytes of the packet. >> >> The size of the skb_pull must therefore also be taken into account when the >> buffer's tailroom is expanded. >> >> Fixes: 610bfc6bc99b ("batman-adv: Receive fragmented packets and merge") >> Reported-by: Martin Weinelt <martin@darmstadt.freifunk.net> >> Co-authored-by: Linus Lüssing <linus.luessing@c0d3.blue> >> Signed-off-by: Sven Eckelmann <sven@narfation.org> >> Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de> >> Signed-off-by: Sasha Levin <sashal@kernel.org> > [...] MBR, Sergei
diff --git a/net/batman-adv/fragmentation.c b/net/batman-adv/fragmentation.c index 0fddc17106bd..5b71a289d04f 100644 --- a/net/batman-adv/fragmentation.c +++ b/net/batman-adv/fragmentation.c @@ -275,7 +275,7 @@ batadv_frag_merge_packets(struct hlist_head *chain) kfree(entry); packet = (struct batadv_frag_packet *)skb_out->data; - size = ntohs(packet->total_size); + size = ntohs(packet->total_size) + hdr_size; /* Make room for the rest of the fragments. */ if (pskb_expand_head(skb_out, 0, size - skb_out->len, GFP_ATOMIC) < 0) {