[PATCH net-next 4/4] 6lowpan: len field is not stored and accessed properly

[PATCH net-next 4/4] 6lowpan: len field is not stored and accessed properly

[Tony Cheneau]

Lenght field should be encoded (and accessed) the other way around.
As it is currently written, it could lead to interroperability issues.

Also, I rewrote the code so that iphc0 argument of
lowpan_alloc_new_frame could be removed.
---
 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 af2f12e..b400156 100644
--- a/net/ieee802154/6lowpan.c
+++ b/net/ieee802154/6lowpan.c
@@ -654,7 +654,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;
 
@@ -665,7 +665,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 */
@@ -721,13 +721,17 @@ 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;
 
-		len = lowpan_fetch_skb_u8(skb); /* frame length */
+		slen = lowpan_fetch_skb_u8(skb); /* frame length */
 		tag = lowpan_fetch_skb_u16(skb);
 
+		/* 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
@@ -742,7 +746,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;
 		}
@@ -1000,8 +1004,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 4/4] 6lowpan: len field is not stored and accessed properly

[Alexander Smirnov]

2012/6/11 Tony Cheneau <tony.cheneauh@amnesiak.org>:
> Lenght field should be encoded (and accessed) the other way around.
> As it is currently written, it could lead to interroperability issues.

What kind of "interroperability issues" can occur? Please describe it
in details, I can't read your mind. And again, can these problems be
caused by the byte-order mismatch?

>
> Also, I rewrote the code so that iphc0 argument of
> lowpan_alloc_new_frame could be removed.
> ---
>  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 af2f12e..b400156 100644
> --- a/net/ieee802154/6lowpan.c
> +++ b/net/ieee802154/6lowpan.c
> @@ -654,7 +654,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;
>
> @@ -665,7 +665,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 */
> @@ -721,13 +721,17 @@ 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;
>
> -               len = lowpan_fetch_skb_u8(skb); /* frame length */
> +               slen = lowpan_fetch_skb_u8(skb); /* frame length */
>                tag = lowpan_fetch_skb_u16(skb);
>
> +               /* 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
> @@ -742,7 +746,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;
>                }
> @@ -1000,8 +1004,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 4/4] 6lowpan: len field is not stored and accessed properly

[Tony Cheneau]

Hi again,

Thank you for your comment. See my answer inline.

Le Tue, 12 Jun 2012 22:20:13 +0400,
Alexander Smirnov <alex.bluesman.smirnov@gmail.com> a écrit :

> 2012/6/11 Tony Cheneau <tony.cheneauh@amnesiak.org>:
> > Lenght field should be encoded (and accessed) the other way around.
> > As it is currently written, it could lead to interroperability
> > issues.
> 
> What kind of "interroperability issues" can occur? Please describe it
> in details, I can't read your mind. And again, can these problems be
> caused by the byte-order mismatch?
Sorry if I wasn't clear. Yes, I believe it is a byte-order mismatch.
I wasn't able to verify interoperability with other implementations
myself, but I compared with some packet capture I obtained from some
other implementations, where the byte-order was different. I'll double
check that when I'll have access to my lab again.
The reason I choose the term "interoperability issues" is that this
patch focuses on the length field where patch 3 focused on the tag
field. If you encode the tag field incorrectly, it is not a big deal,
because the recipient will decode wrongly, but all fragment of a same
packet will share a same tag (regardless of you decode it properly or
not). However, when the same thing happen with the length field, if you
endianness is wrong, the recipient will interpret a totally different
length and will reserve a wrong amount of memory in order to store the
packet (this is mainly a matter of concern if you are performing
fragment reassembly and do not allocate enough memory for the complete
frame). Do that make sense?

Just like the previous patch, I'll rework it as well.

As a side note, do you have any opinions on the first two patches 

Regards,
	Tony



> >
> > Also, I rewrote the code so that iphc0 argument of
> > lowpan_alloc_new_frame could be removed.
> > ---
> >  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 af2f12e..b400156 100644
> > --- a/net/ieee802154/6lowpan.c
> > +++ b/net/ieee802154/6lowpan.c
> > @@ -654,7 +654,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;
> >
> > @@ -665,7 +665,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 */
> > @@ -721,13 +721,17 @@ 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;
> >
> > -               len = lowpan_fetch_skb_u8(skb); /* frame length */
> > +               slen = lowpan_fetch_skb_u8(skb); /* frame length */
> >                tag = lowpan_fetch_skb_u16(skb);
> >
> > +               /* 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
> > @@ -742,7 +746,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;
> >                }
> > @@ -1000,8 +1004,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
> >
> 
> ------------------------------------------------------------------------------
> Live Security Virtual Conference
> Exclusive live event will cover all the ways today's security and 
> threat landscape has changed and how IT managers can respond.
> Discussions will include endpoint security, mobile security and the
> latest in malware threats.
> http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
> _______________________________________________ Linux-zigbee-devel
> mailing list Linux-zigbee-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Linux-zigbee-devel mailing list
Linux-zigbee-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-zigbee-devel