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
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
