Skip to contentSkip to conversation

batman-adv: Calculate extra tail size based on queued fragments

B.A.T.M.A.N. Applied Sven Eckelmann 3 participants 3 messages Dec 1, 2014 – Dec 8, 2014

lore
Share & tools
lore

Sven Eckelmann

author
#0 lore raw

The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606 ("batman-adv: Receive fragmented packets and merge"). The new code provided a mostly unused parameter skb for the merging function. It is used inside the function to calculate the additionally needed skb tailroom. But instead of increasing its own tailroom, it is only increasing the tailroom of the first queued skb. This is not correct in some situations because the first queued entry can be a different one than the parameter.

An observed problem was:

1. packet with size 104, total_size 1464, fragno 1 was received
- packet is queued
2. packet with size 1400, total_size 1464, fragno 0 was received
- packet is queued at the end of the list
3. enough data was received and can be given to the merge function (1464 == (1400 - 20) + (104 - 20))
- merge functions gets 1400 byte large packet as skb argument
4. merge function gets first entry in queue (104 byte)
- stored as skb_out
5. merge function calculates the required extra tail as total_size - skb->len
- pskb_expand_head tail of skb_out with 64 bytes
6. merge function tries to squeeze the extra 1380 bytes from the second queued skb (1400 byte aka skb parameter) in the 64 extra tail bytes of skb_out

Instead calculate the extra required tail bytes for skb_out also using skb_out instead of using the parameter skb. The skb parameter is only used to get the total_size from the last received packet. This is also the total_size used to decide that all fragments were received.

Signed-off-by Sven Eckelmann <[email protected]>
Reported-by Philipp Psurek <[email protected]>

---
This is a minimized version which doesn't require the patch "[PATCH-maint] batman-adv: Check total_size when reassembling fragments".

---
 fragmentation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fragmentation.c +1 -1
index f14e54a..af16844 100644
@@ -247,7 +247,7 @@ batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
 	kfree(entry);
 
 	/* Make room for the rest of the fragments. */
-	if (pskb_expand_head(skb_out, 0, size - skb->len, GFP_ATOMIC) < 0) {
+	if (pskb_expand_head(skb_out, 0, size - skb_out->len, GFP_ATOMIC) < 0) {
 		kfree_skb(skb_out);
 		skb_out = NULL;
 		goto free;
Signature · 1 line
2.1.3
Acked-by
#1 lore raw

Hi Sven,

Thanks for looking into this!

Quoted 44 lines · Sven Eckelmann

The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606 ("batman-adv: Receive fragmented packets and merge"). The new code provided a mostly unused parameter skb for the merging function. It is used inside the function to calculate the additionally needed skb tailroom. But instead of increasing its own tailroom, it is only increasing the tailroom of the first queued skb. This is not correct in some situations because the first queued entry can be a different one than the parameter.

An observed problem was:

1. packet with size 104, total_size 1464, fragno 1 was received
- packet is queued
2. packet with size 1400, total_size 1464, fragno 0 was received
- packet is queued at the end of the list
3. enough data was received and can be given to the merge function (1464 == (1400 - 20) + (104 - 20))
- merge functions gets 1400 byte large packet as skb argument
4. merge function gets first entry in queue (104 byte)
- stored as skb_out
5. merge function calculates the required extra tail as total_size - skb->len
- pskb_expand_head tail of skb_out with 64 bytes
6. merge function tries to squeeze the extra 1380 bytes from the second queued skb (1400 byte aka skb parameter) in the 64 extra tail bytes of skb_out

Instead calculate the extra required tail bytes for skb_out also using skb_out instead of using the parameter skb. The skb parameter is only used to get the total_size from the last received packet. This is also the total_size used to decide that all fragments were received.

Signed-off-by Sven Eckelmann <[email protected]>
Reported-by Philipp Psurek <[email protected]>

---
This is a minimized version which doesn't require the patch "[PATCH-maint] batman-adv: Check total_size when reassembling fragments".

---
  fragmentation.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
fragmentation.c +0 -0
index f14e54a..af16844 100644

fragmentation.c batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb) · line 253

Quoted 8 lines
fragmentation.c +1 -1
@@ -247,7 +247,7 @@ batadv_frag_merge_packets(struct hlist_head *chain, struct sk_buff *skb)
  	kfree(entry);
 
  	/* Make room for the rest of the fragments. */
-	if (pskb_expand_head(skb_out, 0, size - skb->len, GFP_ATOMIC) < 0) {
+	if (pskb_expand_head(skb_out, 0, size - skb_out->len, GFP_ATOMIC) < 0) {
  		kfree_skb(skb_out);
  		skb_out = NULL;
  		goto free;

I noticed the same mistake, when looking into Philipp's issue, but couldn't make wrap my head around it (as to why it should cause the crash). Your description makes sense, and I think my mistake was to forget the inverse tx/rx of fragments.

Anyways:

Acked-by Martin Hundebøll <[email protected]>

+45 61 65 54 61
[email protected]

Applied
#2 lore raw
In reply to Sven Eckelmann
Quoted 31 lines · On Monday 01 December 2014 10:32:10 Sven Eckelmann wrote:

The fragmentation code was replaced in 9b3eab61754d74a93c9840c296013fe3b4a1b606 ("batman-adv: Receive fragmented packets and merge"). The new code provided a mostly unused parameter skb for the merging function. It is used inside the function to calculate the additionally needed skb tailroom. But instead of increasing its own tailroom, it is only increasing the tailroom of the first queued skb. This is not correct in some situations because the first queued entry can be a different one than the parameter.

An observed problem was:

1. packet with size 104, total_size 1464, fragno 1 was received
- packet is queued
2. packet with size 1400, total_size 1464, fragno 0 was received
- packet is queued at the end of the list
3. enough data was received and can be given to the merge function (1464 == (1400 - 20) + (104 - 20))
- merge functions gets 1400 byte large packet as skb argument
4. merge function gets first entry in queue (104 byte)
- stored as skb_out
5. merge function calculates the required extra tail as total_size - skb->len - pskb_expand_head tail of skb_out with 64 bytes
6. merge function tries to squeeze the extra 1380 bytes from the second queued skb (1400 byte aka skb parameter) in the 64 extra tail bytes of skb_out

Instead calculate the extra required tail bytes for skb_out also using skb_out instead of using the parameter skb. The skb parameter is only used to get the total_size from the last received packet. This is also the total_size used to decide that all fragments were received.

Quoted 8 lines
Signed-off-by Sven Eckelmann <[email protected]>
Reported-by Philipp Psurek <[email protected]>

---
This is a minimized version which doesn't require the patch "[PATCH-maint] batman-adv: Check total_size when reassembling fragments".

---
 fragmentation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied in revision 210a3cb.

Thanks,
Marek