[PATCH] 6lowpan: Fix extraction of flow label field

[PATCH] 6lowpan: Fix extraction of flow label field

[Lukasz Duda]

The lowpan_fetch_skb function is used to fetch the first byte,
which also increments the data pointer in skb structure,
making subsequent array lookup of byte 0 actually being byte 1.

To decompress the first byte of the Flow Label when the TF flag is
set to 0x01, the second half of the first byte is needed.

The patch fixes the extraction of the Flow Label field.

Signed-off-by: Lukasz Duda <lukasz.duda@nordicsemi.no>
Signed-off-by: Glenn Ruben Bakke <glenn.ruben.bakke@nordicsemi.no>
---
 net/6lowpan/iphc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 9055d7b..74e56d7 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -284,7 +284,7 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
 		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
 			return -EINVAL;
 
-		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
+		hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);
 		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
 		skb_pull(skb, 2);
 		break;
-- 
2.1.4

Re: [PATCH] 6lowpan: Fix extraction of flow label field

[Alexander Aring]

On Tue, Jun 30, 2015 at 03:24:52AM -0700, Lukasz Duda wrote:
> The lowpan_fetch_skb function is used to fetch the first byte,
> which also increments the data pointer in skb structure,
> making subsequent array lookup of byte 0 actually being byte 1.
> 
> To decompress the first byte of the Flow Label when the TF flag is
> set to 0x01, the second half of the first byte is needed.
> 
> The patch fixes the extraction of the Flow Label field.
> 
> Signed-off-by: Lukasz Duda <lukasz.duda@nordicsemi.no>
> Signed-off-by: Glenn Ruben Bakke <glenn.ruben.bakke@nordicsemi.no>

Acked-by: Alexander Aring <alex.aring@gmail.com>

> ---
>  net/6lowpan/iphc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 9055d7b..74e56d7 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -284,7 +284,7 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
>  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
>  			return -EINVAL;
>  
> -		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
> +		hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);
>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
>  		skb_pull(skb, 2);
>  		break;

This code part is really hard to decrypt/understand. Nevertheless for
some historical(contiki) reasons we have this situation mainline now
and I had no time to cleanup this code. Actual it remembers me on the
early state of the iphc code.

I would recommended to cleanup the whole traffic flow decompress/compress
part (Maybe also with some magic numbers defines which is used on both sides
and some static inline functions). Really don't like the actually stuff there.
Anyway thank you for dig into this issue now.

One reason why definitly something goes wrong there is that skb->data[0]
information is used for hdr.flow_lbl[1] and hdr.flow_lbl[0] which can't
be. I didn't test it yet and trust you that this will do the right
behaviour for now. I also have no time that I can write really a testcase
for that. What I can say currently is that something is wrong here
and your good looks fine for me and makes more sense than the current behaviour.

Thanks.

- Alex

Re: [PATCH] 6lowpan: Fix extraction of flow label field

[Alexander Aring]

Hi,

On Tue, Jun 30, 2015 at 03:24:52AM -0700, Lukasz Duda wrote:
...
> ---
>  net/6lowpan/iphc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 9055d7b..74e56d7 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -284,7 +284,7 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
>  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
>  			return -EINVAL;
>  
> -		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
> +		hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);
>  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
>  		skb_pull(skb, 2);

Additional note what I see in this code segment. We don't check if we
_can_ pull from the skb. We should here use lowpan_fetch_skb and check
for error like above instead of memcpy and skb_pull.

Maybe simple do the following:

if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)) ||
    lowpan_fetch_skb(skb, &hdr.flow_lbl[1], 2)
	return -EINVAL;

hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);


The issue is that we don't look at:

memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);

if &skb->data[0] data has room of 2 bytes to read from it.

In case of 802.15.4 6LoWPAN, some drivers allocs the exact len value of
the frame (which is even a bad behaviour, they should alloc the MTU
size), somebody could send a 802.15.4 6LoWPAN packet and the frame
looks _at_ _first_ like a 6LoWPAN frame but the lengths field was too
small that an 6LoWPAN frame with this inline data fits in. Means
somebody generated a 6LoWPAN frame that the behaviour occurs to read out
of buffer of skb->data[0].

A good 6LoWPAN stack should not send such frames, but it is possible for
everybody to send something like that which ends in this bad
behaviour in our side.

In case of bluetooth: I have no idea, how skb room is allocated in
bluetooth.


To be sure we need to check if the &skb->data[0] has the buffer length
before to read the amount of bytes (in this case: 2). This is what
lowpan_fetch_skb does by calling the "pskb_may_pull" function before.

-> So I am really not happy about the current solution about this
function. I hope it was understandable what I think we need to do in
this code segment that the stack is more robust to something like that.

I did never touch the flow_lbl code, but I already changed the most code
that we use lowpan_fetch_skb, but there are still some lacks like in this
case. Would be very happy if somebody sends patches for this and should
be part of the cleanup of flow_lbl compression/decompression. :-/

- Alex

Re: [PATCH] 6lowpan: Fix extraction of flow label field

[Stefan Schmidt]

Hello.

On 30/06/15 12:24, Lukasz Duda wrote:
> The lowpan_fetch_skb function is used to fetch the first byte,
> which also increments the data pointer in skb structure,
> making subsequent array lookup of byte 0 actually being byte 1.
>
> To decompress the first byte of the Flow Label when the TF flag is
> set to 0x01, the second half of the first byte is needed.
>
> The patch fixes the extraction of the Flow Label field.
>
> Signed-off-by: Lukasz Duda <lukasz.duda@nordicsemi.no>
> Signed-off-by: Glenn Ruben Bakke <glenn.ruben.bakke@nordicsemi.no>
> ---
>   net/6lowpan/iphc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 9055d7b..74e56d7 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -284,7 +284,7 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
>   		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
>   			return -EINVAL;
>   
> -		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
> +		hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);
>   		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
>   		skb_pull(skb, 2);
>   		break;

Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>

Alex, I agree that this area needs some improvements. I would still like 
to see this simple and obvious bug fix to go in now even if it gets 
changed with a refactor later on. Better have this bug fixed now. :) You 
acked it already so I think it is fine to go in? Marcel did not apply it 
yet.

regards
Stefan Schmidt

Re: [PATCH] 6lowpan: Fix extraction of flow label field

[Alexander Aring]

On Thu, Jul 30, 2015 at 10:03:37PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 30/06/15 12:24, Lukasz Duda wrote:
> >The lowpan_fetch_skb function is used to fetch the first byte,
> >which also increments the data pointer in skb structure,
> >making subsequent array lookup of byte 0 actually being byte 1.
> >
> >To decompress the first byte of the Flow Label when the TF flag is
> >set to 0x01, the second half of the first byte is needed.
> >
> >The patch fixes the extraction of the Flow Label field.
> >
> >Signed-off-by: Lukasz Duda <lukasz.duda@nordicsemi.no>
> >Signed-off-by: Glenn Ruben Bakke <glenn.ruben.bakke@nordicsemi.no>
> >---
> >  net/6lowpan/iphc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> >index 9055d7b..74e56d7 100644
> >--- a/net/6lowpan/iphc.c
> >+++ b/net/6lowpan/iphc.c
> >@@ -284,7 +284,7 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
> >  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
> >  			return -EINVAL;
> >-		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
> >+		hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);
> >  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
> >  		skb_pull(skb, 2);
> >  		break;
> 
> Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>
> 
> Alex, I agree that this area needs some improvements. I would still like to
> see this simple and obvious bug fix to go in now even if it gets changed
> with a refactor later on. Better have this bug fixed now. :) You acked it

I need to admit, I refactor this function multiple times and never had
time (or maybe I simple not did it, don't know why) to sending patches
for it. There is too also much things which can be optimized in the whole
iphc code. :-/

Also I remember 2-3 workarounds which we have inside IEEE 802.15.4
6LoWPAN, also having inside my queue 802.15.4 security layer and such
things. At the moment not a good idea to deal with all at the same time.

> already so I think it is fine to go in? Marcel did not apply it yet.
> 

Yes, I forget that patch and Marcel asked me today what's inside the
queue. So the pull request for net-next is already out, but I think we
can live to having this patch in the next one.

Marcel can you please apply this patch?

Jukka you are also fine with this patch? Please send your Acked-by then.

Thanks.

- Alex

Re: [PATCH] 6lowpan: Fix extraction of flow label field

[Jukka Rissanen]

On ti, 2015-06-30 at 23:22 +0200, Alexander Aring wrote:
> On Tue, Jun 30, 2015 at 03:24:52AM -0700, Lukasz Duda wrote:
> > The lowpan_fetch_skb function is used to fetch the first byte,
> > which also increments the data pointer in skb structure,
> > making subsequent array lookup of byte 0 actually being byte 1.
> > 
> > To decompress the first byte of the Flow Label when the TF flag is
> > set to 0x01, the second half of the first byte is needed.
> > 
> > The patch fixes the extraction of the Flow Label field.
> > 
> > Signed-off-by: Lukasz Duda <lukasz.duda@nordicsemi.no>
> > Signed-off-by: Glenn Ruben Bakke <glenn.ruben.bakke@nordicsemi.no>
> 
> Acked-by: Alexander Aring <alex.aring@gmail.com>

Acked-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>

> 
> > ---
> >  net/6lowpan/iphc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> > index 9055d7b..74e56d7 100644
> > --- a/net/6lowpan/iphc.c
> > +++ b/net/6lowpan/iphc.c
> > @@ -284,7 +284,7 @@ lowpan_header_decompress(struct sk_buff *skb, struct net_device *dev,
> >  		if (lowpan_fetch_skb(skb, &tmp, sizeof(tmp)))
> >  			return -EINVAL;
> >  
> > -		hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30);
> > +		hdr.flow_lbl[0] = (tmp & 0x0F) | ((tmp >> 2) & 0x30);
> >  		memcpy(&hdr.flow_lbl[1], &skb->data[0], 2);
> >  		skb_pull(skb, 2);
> >  		break;
> 
> This code part is really hard to decrypt/understand. Nevertheless for
> some historical(contiki) reasons we have this situation mainline now
> and I had no time to cleanup this code. Actual it remembers me on the
> early state of the iphc code.
> 
> I would recommended to cleanup the whole traffic flow decompress/compress
> part (Maybe also with some magic numbers defines which is used on both sides
> and some static inline functions). Really don't like the actually stuff there.
> Anyway thank you for dig into this issue now.
> 
> One reason why definitly something goes wrong there is that skb->data[0]
> information is used for hdr.flow_lbl[1] and hdr.flow_lbl[0] which can't
> be. I didn't test it yet and trust you that this will do the right
> behaviour for now. I also have no time that I can write really a testcase
> for that. What I can say currently is that something is wrong here
> and your good looks fine for me and makes more sense than the current behaviour.
> 
> Thanks.
> 
> - Alex


Cheers,
Jukka