diff mbox

[1/2,v2] ieee802154: assembly of 6LoWPAN fragments improvement

Message ID 20180717152546.GA22664@Rafael-Mac.intra.sownet.nl (mailing list archive)
State Under Review
Delegated to: Stefan Schmidt
Headers show

Commit Message

Rafael Vuijk July 17, 2018, 3:25 p.m. UTC 
6LoWPAN reassembly length check matching first known packet length.

Signed-off-by: Rafael Vuijk <r.vuijk@sownet.nl>

--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stefan Schmidt July 24, 2018, 1 p.m. UTC | #1 
Hello.

On 17.07.2018 17:25, Rafael Vuijk wrote:
> 6LoWPAN reassembly length check matching first known packet length.


Your original commit log had way more information on the problem.

"We have tested the 6LoWPAN modules in the Linux kernel and came to some
issue regarding fragmentation. We have seen aborted SCP transfers
("message authentication code incorrect") and tested TCP transfers as
well and saw corruption on fragment-sized intervals. The current
fragment assembling functions do not check enough for corrupted L2
packets that might slip through L2 CRC check. (in our case IEEE802.15.4
which has only 16-bit CRC).
As a result, overlapping fragments due to offset corruption are not
detected and assembled incorrectly. Part of packets may have old data.
At TCP-level, there is only a simple TCP-checksum which is not enough in
this case as the corruption occurs frequently (once every few minutes).

After quickly analysing the code we saw some potential issues and
created a patch that adds additional overlap checks and simplifies some
conditional statements. After running tests again, TCP corruption was
not seen again. The test was performed with SCP and keeps transferring
large files now without error."

It would be great if some of this finds it way into the commit log of
these two patches. At a minimum I would require them to not have the
same commit summary line.


> Signed-off-by: Rafael Vuijk <r.vuijk@sownet.nl>
> --- ./net/ieee802154/6lowpan/reassembly.c	2018-02-20 11:10:06.000000000 +0100
> +++ ./net/ieee802154/6lowpan/reassembly.c	2018-02-21 09:13:29.000000000 +0100
> @@ -140,23 +140,14 @@ static int lowpan_frag_queue(struct lowp
>  	offset = lowpan_802154_cb(skb)->d_offset << 3;
>  	end = lowpan_802154_cb(skb)->d_size;
>  
> +	if (fq->q.len == 0)
> +		fq->q.len = end;
> +	if (fq->q.len != end)
> +		goto err;
> +
>  	/* Is this the final fragment? */
>  	if (offset + skb->len == end) {
> -		/* If we already have some bits beyond end
> -		 * or have different end, the segment is corrupted.
> -		 */
> -		if (end < fq->q.len ||
> -		    ((fq->q.flags & INET_FRAG_LAST_IN) && end != fq->q.len))
> -			goto err;
>  		fq->q.flags |= INET_FRAG_LAST_IN;
> -		fq->q.len = end;
> -	} else {
> -		if (end > fq->q.len) {
> -			/* Some bits beyond end -> corruption. */
> -			if (fq->q.flags & INET_FRAG_LAST_IN)
> -				goto err;
> -			fq->q.len = end;
> -		}

Some basic testing on my side has not revealed any issues on this. I
will give it some longer testing with SCP now.

regards
Stefan Schmidt
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Richardson July 24, 2018, 3:20 p.m. UTC | #2 
Stefan Schmidt <stefan@datenfreihafen.org> wrote:
    > "We have tested the 6LoWPAN modules in the Linux kernel and came to some
    > issue regarding fragmentation. We have seen aborted SCP transfers
    > ("message authentication code incorrect") and tested TCP transfers as
    > well and saw corruption on fragment-sized intervals. The current
    > fragment assembling functions do not check enough for corrupted L2
    > packets that might slip through L2 CRC check. (in our case IEEE802.15.4
    > which has only 16-bit CRC).
    > As a result, overlapping fragments due to offset corruption are not
    > detected and assembled incorrectly. Part of packets may have old data.
    > At TCP-level, there is only a simple TCP-checksum which is not enough in
    > this case as the corruption occurs frequently (once every few minutes).

This is a common problem with checksums, fragments and "high" bandwidths.
It can occur with some frequency, and has been a reason for much of the PMTU
work (including PLMTUD) to get rid of IP-level fragmentation.
A solution is better checksums: or rather integrity hashes.

I'm curious if the problem would have been if the 802.15.4 network was
encrypted, and thus had an cryptographic integrity check.

    > After quickly analysing the code we saw some potential issues and
    > created a patch that adds additional overlap checks and simplifies some
    > conditional statements. After running tests again, TCP corruption was
    > not seen again. The test was performed with SCP and keeps transferring
    > large files now without error."

    > It would be great if some of this finds it way into the commit log of
    > these two patches. At a minimum I would require them to not have the
    > same commit summary line.

Agreed.

    >> Signed-off-by: Rafael Vuijk <r.vuijk@sownet.nl>
    >> --- ./net/ieee802154/6lowpan/reassembly.c	2018-02-20 11:10:06.000000000 +0100
    >> +++ ./net/ieee802154/6lowpan/reassembly.c	2018-02-21 09:13:29.000000000 +0100
    >> @@ -140,23 +140,14 @@ static int lowpan_frag_queue(struct lowp
    >> offset = lowpan_802154_cb(skb)->d_offset << 3;
    >> end = lowpan_802154_cb(skb)->d_size;
    >> 
    >> +	if (fq->q.len == 0)
    >> +		fq->q.len = end;
    >> +	if (fq->q.len != end)
    >> +		goto err;
    >> +
    >> /* Is this the final fragment? */
    >> if (offset + skb->len == end) {
    >> -		/* If we already have some bits beyond end
    >> -		 * or have different end, the segment is corrupted.
    >> -		 */
    >> -		if (end < fq->q.len ||
    >> -		    ((fq->q.flags & INET_FRAG_LAST_IN) && end != fq->q.len))
    >> -			goto err;
    fq-> q.flags |= INET_FRAG_LAST_IN;
    >> -		fq->q.len = end;
    >> -	} else {
    >> -		if (end > fq->q.len) {
    >> -			/* Some bits beyond end -> corruption. */
    >> -			if (fq->q.flags & INET_FRAG_LAST_IN)
    >> -				goto err;
    >> -			fq->q.len = end;
    >> -		}

    > Some basic testing on my side has not revealed any issues on this. I
    > will give it some longer testing with SCP now.

    > regards
    > Stefan Schmidt
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- ./net/ieee802154/6lowpan/reassembly.c	2018-02-20 11:10:06.000000000 +0100
+++ ./net/ieee802154/6lowpan/reassembly.c	2018-02-21 09:13:29.000000000 +0100
@@ -140,23 +140,14 @@  static int lowpan_frag_queue(struct lowp
 	offset = lowpan_802154_cb(skb)->d_offset << 3;
 	end = lowpan_802154_cb(skb)->d_size;
 
+	if (fq->q.len == 0)
+		fq->q.len = end;
+	if (fq->q.len != end)
+		goto err;
+
 	/* Is this the final fragment? */
 	if (offset + skb->len == end) {
-		/* If we already have some bits beyond end
-		 * or have different end, the segment is corrupted.
-		 */
-		if (end < fq->q.len ||
-		    ((fq->q.flags & INET_FRAG_LAST_IN) && end != fq->q.len))
-			goto err;
 		fq->q.flags |= INET_FRAG_LAST_IN;
-		fq->q.len = end;
-	} else {
-		if (end > fq->q.len) {
-			/* Some bits beyond end -> corruption. */
-			if (fq->q.flags & INET_FRAG_LAST_IN)
-				goto err;
-			fq->q.len = end;
-		}
 	}
 
 	/* Find out which fragments are in front and at the back of us