[PATCH net-next v2 0/4] 6lowpan: Various bug fixes

[PATCH net-next v2 0/4] 6lowpan: Various bug fixes

[Tony Cheneau]

Hello,

After reading and playing with the 6lowpan code, I found out a few issues. This
patchset fixes them. This patchset should apply cleanly against the current
net-next. It contains only bug fixes, I'll send later on an other patchset that
will contain new functionalities.

Alexander commented on the previous version of this patchset and made me
understand that the commit messages were not specific enough. This new version
hopefully improves that.

This is a set of 4 small patches that correct bugs in 6lowpan:
- patch 1 fixes a potential crash when reassembling UDP fragments
- patch 2 fixes a type length issues that prevent the fragmentation reassembly
  to operate properly.
- patch 3 and 4 corrects field encoding issues (byte order was not right)

Hope it helps.

Regards,
	Tony Cheneau

 net/ieee802154/6lowpan.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

-- 
1.7.3.4

[PATCH net-next v2 1/4] Fix null pointer dereference in UDP uncompression function

[Tony Cheneau]

When a UDP packet gets fragmented, a crash will occur at reassembly time.
This is because skb->transport_header is not set during earlier period of fragment reassembly.
As a consequence, call to udp_hdr() return NULL and uh (which is NULL) gets
dereferenced without much test.

---
 net/ieee802154/6lowpan.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index f4070e5..07fa200 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -314,6 +314,9 @@ lowpan_uncompress_udp_header(struct sk_buff *skb)
 {
 	struct udphdr *uh = udp_hdr(skb);
 	u8 tmp;
+
+	if (!uh)
+		goto err;
 
 	if (lowpan_fetch_skb_u8(skb, &tmp))
 		goto err;
-- 
1.7.3.4

[PATCH net-next v2 2/4] Incorrect type length in lowpan_alloc_new_frame():tag is u8 but should be u16

[Tony Cheneau]

Function lowpan_alloc_new_frame() takes u8 tag as an argument. However,
its only caller, lowpan_process_data() passes down a u16. Hence,
the tag value can get corrupted. This prevent 6lowpan fragment reassembly of a
message when the fragment tag value is over 256.

---
 net/ieee802154/6lowpan.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index 07fa200..5884d29 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -649,7 +649,7 @@ static void lowpan_fragment_timer_expired(unsigned long entry_addr)
 }
 
 static struct lowpan_fragment *
-lowpan_alloc_new_frame(struct sk_buff *skb, u8 iphc0, u8 len, u8 tag)
+lowpan_alloc_new_frame(struct sk_buff *skb, u8 iphc0, u8 len, u16 tag)
 {
 	struct lowpan_fragment *frame;
 
-- 
1.7.3.4

[PATCH net-next v2 3/4] Change byte order when storing/accessing tag field

[Tony Cheneau]

The tag field should be stored and accessed using big endian byte order (as
intended in the specs). Or else, when displayed with a trafic analyser, such a
Wireshark, the field not properly displayed (e.g. 0x01 00 instead of 0x00 01).

---
 net/ieee802154/6lowpan.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index 5884d29..b886e07 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -303,7 +303,7 @@ static inline int lowpan_fetch_skb_u16(struct sk_buff *skb, u16 *val)
 	if (unlikely(!pskb_may_pull(skb, 2)))
 		return -EINVAL;
 
-	*val = skb->data[0] | (skb->data[1] << 8);
+	*val = (skb->data[0] << 8) | skb->data[1];
 	skb_pull(skb, 2);
 
 	return 0;
@@ -1010,8 +1010,8 @@ lowpan_skb_fragmentation(struct sk_buff *skb)
 	/* first fragment header */
 	head[0] = LOWPAN_DISPATCH_FRAG1 | (payload_length & 0x7);
 	head[1] = (payload_length >> 3) & 0xff;
-	head[2] = tag & 0xff;
-	head[3] = tag >> 8;
+	head[2] = tag >> 8;
+	head[3] = tag & 0xff;
 
 	err = lowpan_fragment_xmit(skb, head, header_length, 0, 0);
 
-- 
1.7.3.4

[PATCH net-next v2 4/4] Change byte order when storing/accessing len field

[Tony Cheneau]

Length field should be encoded using big endian byte order, such as intend by
the specs.
As it is currently written, the len field would not be decoded properly on an
implementation following the correct byte ordering. Hence, it could lead to
interoperability issues.

Also, code has been rewritten so that iphc0 argument of lowpan_alloc_new_frame()
is no longer necessary.

---
 net/ieee802154/6lowpan.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c
index b886e07..33a4093 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -649,7 +649,7 @@ static void lowpan_fragment_timer_expired(unsigned long entry_addr)
 }
 
 static struct lowpan_fragment *
-lowpan_alloc_new_frame(struct sk_buff *skb, u8 iphc0, u8 len, u16 tag)
+lowpan_alloc_new_frame(struct sk_buff *skb, u16 len, u16 tag)
 {
 	struct lowpan_fragment *frame;
 
@@ -660,7 +660,7 @@ lowpan_alloc_new_frame(struct sk_buff *skb, u8 iphc0, u8 len, u16 tag)
 
 	INIT_LIST_HEAD(&frame->list);
 
-	frame->length = (iphc0 & 7) | (len << 3);
+	frame->length = len;
 	frame->tag = tag;
 
 	/* allocate buffer for frame assembling */
@@ -718,14 +718,18 @@ lowpan_process_data(struct sk_buff *skb)
 	case LOWPAN_DISPATCH_FRAGN:
 	{
 		struct lowpan_fragment *frame;
-		u8 len, offset;
-		u16 tag;
+		/* slen stores the rightmost 8 bits of the 11 bits length */
+		u8 slen, offset;
+		u16 len, tag;
 		bool found = false;
 
-		if (lowpan_fetch_skb_u8(skb, &len) || /* frame length */
+		if (lowpan_fetch_skb_u8(skb, &slen) || /* frame length */
 		    lowpan_fetch_skb_u16(skb, &tag))  /* fragment tag */
 			goto drop;
 
+		/* adds the 3 MSB to the 8 LSB to retrieve the 11 bits length */
+		len = ((iphc0 & 7) << 8) | slen;
+
 		/*
 		 * check if frame assembling with the same tag is
 		 * already in progress
@@ -740,7 +744,7 @@ lowpan_process_data(struct sk_buff *skb)
 
 		/* alloc new frame structure */
 		if (!found) {
-			frame = lowpan_alloc_new_frame(skb, iphc0, len, tag);
+			frame = lowpan_alloc_new_frame(skb, len, tag);
 			if (!frame)
 				goto unlock_and_drop;
 		}
@@ -1008,8 +1012,8 @@ lowpan_skb_fragmentation(struct sk_buff *skb)
 	tag = fragment_tag++;
 
 	/* first fragment header */
-	head[0] = LOWPAN_DISPATCH_FRAG1 | (payload_length & 0x7);
-	head[1] = (payload_length >> 3) & 0xff;
+	head[0] = LOWPAN_DISPATCH_FRAG1 | ((payload_length >> 8) & 0x7);
+	head[1] = payload_length & 0xff;
 	head[2] = tag >> 8;
 	head[3] = tag & 0xff;
 
-- 
1.7.3.4

Re: [PATCH net-next v2 0/4] 6lowpan: Various bug fixes

[Alexander Smirnov]

> After reading and playing with the 6lowpan code, I found out a few issues. This
> patchset fixes them. This patchset should apply cleanly against the current
> net-next. It contains only bug fixes, I'll send later on an other patchset that
> will contain new functionalities.
>
> Alexander commented on the previous version of this patchset and made me
> understand that the commit messages were not specific enough. This new version
> hopefully improves that.
>
> This is a set of 4 small patches that correct bugs in 6lowpan:
> - patch 1 fixes a potential crash when reassembling UDP fragments
> - patch 2 fixes a type length issues that prevent the fragmentation reassembly
>   to operate properly.
> - patch 3 and 4 corrects field encoding issues (byte order was not right)
>
> Hope it helps.

Looks good.

Just one note. Please add the project name into the header of your
patches, something like:

6lowpan: blablabla

Re: [PATCH net-next v2 0/4] 6lowpan: Various bug fixes

[David Miller]

You've provided no signoffs in your commit messages, please fix this up
and resubmit this entire series.