* [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check [not found] <1336656163-19382-1-git-send-email-y> @ 2012-05-10 13:22 ` alex.bluesman.smirnov 2012-05-10 13:30 ` Eric Dumazet 2012-05-11 14:58 ` [PATCH 1/2 net v2] 6lowpan: rework data fetching from skb Alexander Smirnov 2012-05-10 13:22 ` [PATCH 2/2 net] 6lowpan: fix hop limit compression alex.bluesman.smirnov 1 sibling, 2 replies; 7+ messages in thread From: alex.bluesman.smirnov @ 2012-05-10 13:22 UTC (permalink / raw) To: davem; +Cc: netdev, Alexander Smirnov From: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> Add pskb_may_pull() call when fetching u8 from skb. Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> --- net/ieee802154/6lowpan.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c index 32eb417..0ab3efe 100644 --- a/net/ieee802154/6lowpan.c +++ b/net/ieee802154/6lowpan.c @@ -295,6 +295,8 @@ static u8 lowpan_fetch_skb_u8(struct sk_buff *skb) { u8 ret; + BUG_ON(!pskb_may_pull(skb, 1)); + ret = skb->data[0]; skb_pull(skb, 1); -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check 2012-05-10 13:22 ` [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check alex.bluesman.smirnov @ 2012-05-10 13:30 ` Eric Dumazet 2012-05-10 14:05 ` Alexander Smirnov 2012-05-11 14:58 ` [PATCH 1/2 net v2] 6lowpan: rework data fetching from skb Alexander Smirnov 1 sibling, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2012-05-10 13:30 UTC (permalink / raw) To: alex.bluesman.smirnov; +Cc: davem, netdev On Thu, 2012-05-10 at 17:22 +0400, alex.bluesman.smirnov@gmail.com wrote: > From: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> > > Add pskb_may_pull() call when fetching u8 from skb. > > Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> > --- > net/ieee802154/6lowpan.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c > index 32eb417..0ab3efe 100644 > --- a/net/ieee802154/6lowpan.c > +++ b/net/ieee802154/6lowpan.c > @@ -295,6 +295,8 @@ static u8 lowpan_fetch_skb_u8(struct sk_buff *skb) > { > u8 ret; > > + BUG_ON(!pskb_may_pull(skb, 1)); > + > ret = skb->data[0]; > skb_pull(skb, 1); > No, you cant do that. pskb_may_pull() can fail, and you crash your machine instead of graceful error reporting. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check 2012-05-10 13:30 ` Eric Dumazet @ 2012-05-10 14:05 ` Alexander Smirnov 2012-05-10 16:28 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Alexander Smirnov @ 2012-05-10 14:05 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev Hi Eric, 2012/5/10 Eric Dumazet <eric.dumazet@gmail.com>: > On Thu, 2012-05-10 at 17:22 +0400, alex.bluesman.smirnov@gmail.com > wrote: >> From: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> >> >> Add pskb_may_pull() call when fetching u8 from skb. >> >> Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> >> --- >> net/ieee802154/6lowpan.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c >> index 32eb417..0ab3efe 100644 >> --- a/net/ieee802154/6lowpan.c >> +++ b/net/ieee802154/6lowpan.c >> @@ -295,6 +295,8 @@ static u8 lowpan_fetch_skb_u8(struct sk_buff *skb) >> { >> u8 ret; >> >> + BUG_ON(!pskb_may_pull(skb, 1)); >> + >> ret = skb->data[0]; >> skb_pull(skb, 1); >> > > No, you cant do that. > > pskb_may_pull() can fail, and you crash your machine instead of graceful > error reporting. > thanks for the comment! Using BUG() macro I just want to indicate that something in the bottom of the stack went terribly wrong and you must check your code for bugs.. > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check 2012-05-10 14:05 ` Alexander Smirnov @ 2012-05-10 16:28 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2012-05-10 16:28 UTC (permalink / raw) To: alex.bluesman.smirnov; +Cc: eric.dumazet, netdev From: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> Date: Thu, 10 May 2012 18:05:46 +0400 > Using BUG() macro I just want to indicate that something in the bottom > of the stack went terribly wrong and you must check your code for > bugs.. Then you should do something like: if (WARN_ON_ONCE(!pskb_may_pull(...))) { appropriate_error_handling(); return; } instead. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2 net v2] 6lowpan: rework data fetching from skb 2012-05-10 13:22 ` [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check alex.bluesman.smirnov 2012-05-10 13:30 ` Eric Dumazet @ 2012-05-11 14:58 ` Alexander Smirnov 2012-05-13 3:28 ` David Miller 1 sibling, 1 reply; 7+ messages in thread From: Alexander Smirnov @ 2012-05-11 14:58 UTC (permalink / raw) To: davem; +Cc: netdev, eric.dumazet, Alexander Smirnov This patch reworks functions responsible for fetching data from skb. Now they work more accurately and can notify if something went wrong. Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> --- net/ieee802154/6lowpan.c | 75 ++++++++++++++++++++++++++------------------- 1 files changed, 43 insertions(+), 32 deletions(-) diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c index 32eb417..c2bbf01 100644 --- a/net/ieee802154/6lowpan.c +++ b/net/ieee802154/6lowpan.c @@ -291,25 +291,31 @@ lowpan_compress_udp_header(u8 **hc06_ptr, struct sk_buff *skb) *hc06_ptr += 2; } -static u8 lowpan_fetch_skb_u8(struct sk_buff *skb) +static inline int lowpan_fetch_skb_u8(struct sk_buff *skb, u8 *val) { - u8 ret; + if (WARN_ON_ONCE(!pskb_may_pull(skb, 1))) { + /* + * Uhhh, something went terribly wrong. + * Check the bottom layers code + */ + return -EINVAL; + } - ret = skb->data[0]; + *val = skb->data[0]; skb_pull(skb, 1); - return ret; + return 0; } -static u16 lowpan_fetch_skb_u16(struct sk_buff *skb) +static inline int lowpan_fetch_skb_u16(struct sk_buff *skb, u16 *val) { - u16 ret; - - BUG_ON(!pskb_may_pull(skb, 2)); + if (WARN_ON_ONCE(!pskb_may_pull(skb, 2))) + return -EINVAL; - ret = skb->data[0] | (skb->data[1] << 8); + *val = skb->data[0] | (skb->data[1] << 8); skb_pull(skb, 2); - return ret; + + return 0; } static int @@ -318,7 +324,8 @@ lowpan_uncompress_udp_header(struct sk_buff *skb) struct udphdr *uh = udp_hdr(skb); u8 tmp; - tmp = lowpan_fetch_skb_u8(skb); + if (lowpan_fetch_skb_u8(skb, &tmp)) + goto err; if ((tmp & LOWPAN_NHC_UDP_MASK) == LOWPAN_NHC_UDP_ID) { pr_debug("(%s): UDP header uncompression\n", __func__); @@ -710,7 +717,9 @@ lowpan_process_data(struct sk_buff *skb) /* at least two bytes will be used for the encoding */ if (skb->len < 2) goto drop; - iphc0 = lowpan_fetch_skb_u8(skb); + + if (lowpan_fetch_skb_u8(skb, &iphc0)) + goto drop; /* fragments assembling */ switch (iphc0 & LOWPAN_DISPATCH_MASK) { @@ -722,8 +731,9 @@ lowpan_process_data(struct sk_buff *skb) u16 tag; bool found = false; - len = lowpan_fetch_skb_u8(skb); /* frame length */ - tag = lowpan_fetch_skb_u16(skb); + if (lowpan_fetch_skb_u8(skb, &len) || /* frame length */ + lowpan_fetch_skb_u16(skb, &tag)) /* fragment tag */ + goto drop; /* * check if frame assembling with the same tag is @@ -747,7 +757,8 @@ lowpan_process_data(struct sk_buff *skb) if ((iphc0 & LOWPAN_DISPATCH_MASK) == LOWPAN_DISPATCH_FRAG1) goto unlock_and_drop; - offset = lowpan_fetch_skb_u8(skb); /* fetch offset */ + if (lowpan_fetch_skb_u8(skb, &offset)) /* fetch offset */ + goto unlock_and_drop; /* if payload fits buffer, copy it */ if (likely((offset * 8 + skb->len) <= frame->length)) @@ -769,7 +780,10 @@ lowpan_process_data(struct sk_buff *skb) dev_kfree_skb(skb); skb = frame->skb; kfree(frame); - iphc0 = lowpan_fetch_skb_u8(skb); + + if (lowpan_fetch_skb_u8(skb, &iphc0)) + goto unlock_and_drop; + break; } spin_unlock(&flist_lock); @@ -780,7 +794,8 @@ lowpan_process_data(struct sk_buff *skb) break; } - iphc1 = lowpan_fetch_skb_u8(skb); + if (lowpan_fetch_skb_u8(skb, &iphc1)) + goto drop; _saddr = mac_cb(skb)->sa.hwaddr; _daddr = mac_cb(skb)->da.hwaddr; @@ -791,9 +806,8 @@ lowpan_process_data(struct sk_buff *skb) if (iphc1 & LOWPAN_IPHC_CID) { pr_debug("(%s): CID flag is set, increase header with one\n", __func__); - if (!skb->len) + if (lowpan_fetch_skb_u8(skb, &num_context)) goto drop; - num_context = lowpan_fetch_skb_u8(skb); } hdr.version = 6; @@ -805,9 +819,9 @@ lowpan_process_data(struct sk_buff *skb) * ECN + DSCP + 4-bit Pad + Flow Label (4 bytes) */ case 0: /* 00b */ - if (!skb->len) + if (lowpan_fetch_skb_u8(skb, &tmp)) goto drop; - tmp = lowpan_fetch_skb_u8(skb); + memcpy(&hdr.flow_lbl, &skb->data[0], 3); skb_pull(skb, 3); hdr.priority = ((tmp >> 2) & 0x0f); @@ -819,9 +833,9 @@ lowpan_process_data(struct sk_buff *skb) * ECN + DSCP (1 byte), Flow Label is elided */ case 1: /* 10b */ - if (!skb->len) + if (lowpan_fetch_skb_u8(skb, &tmp)) goto drop; - tmp = lowpan_fetch_skb_u8(skb); + hdr.priority = ((tmp >> 2) & 0x0f); hdr.flow_lbl[0] = ((tmp << 6) & 0xC0) | ((tmp >> 2) & 0x30); hdr.flow_lbl[1] = 0; @@ -832,9 +846,9 @@ lowpan_process_data(struct sk_buff *skb) * ECN + 2-bit Pad + Flow Label (3 bytes), DSCP is elided */ case 2: /* 01b */ - if (!skb->len) + if (lowpan_fetch_skb_u8(skb, &tmp)) goto drop; - tmp = lowpan_fetch_skb_u8(skb); + hdr.flow_lbl[0] = (skb->data[0] & 0x0F) | ((tmp >> 2) & 0x30); memcpy(&hdr.flow_lbl[1], &skb->data[0], 2); skb_pull(skb, 2); @@ -853,9 +867,9 @@ lowpan_process_data(struct sk_buff *skb) /* Next Header */ if ((iphc0 & LOWPAN_IPHC_NH_C) == 0) { /* Next header is carried inline */ - if (!skb->len) + if (lowpan_fetch_skb_u8(skb, &(hdr.nexthdr))) goto drop; - hdr.nexthdr = lowpan_fetch_skb_u8(skb); + pr_debug("(%s): NH flag is set, next header is carried " "inline: %02x\n", __func__, hdr.nexthdr); } @@ -864,9 +878,8 @@ lowpan_process_data(struct sk_buff *skb) if ((iphc0 & 0x03) != LOWPAN_IPHC_TTL_I) hdr.hop_limit = lowpan_ttl_values[iphc0 & 0x03]; else { - if (!skb->len) + if (lowpan_fetch_skb_u8(skb, &(hdr.hop_limit))) goto drop; - hdr.hop_limit = lowpan_fetch_skb_u8(skb); } /* Extract SAM to the tmp variable */ @@ -894,10 +907,8 @@ lowpan_process_data(struct sk_buff *skb) pr_debug("(%s): destination address non-context-based" " multicast compression\n", __func__); if (0 < tmp && tmp < 3) { - if (!skb->len) + if (lowpan_fetch_skb_u8(skb, &prefix[1])) goto drop; - else - prefix[1] = lowpan_fetch_skb_u8(skb); } err = lowpan_uncompress_addr(skb, &hdr.daddr, prefix, -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2 net v2] 6lowpan: rework data fetching from skb 2012-05-11 14:58 ` [PATCH 1/2 net v2] 6lowpan: rework data fetching from skb Alexander Smirnov @ 2012-05-13 3:28 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2012-05-13 3:28 UTC (permalink / raw) To: alex.bluesman.smirnov; +Cc: netdev, eric.dumazet This and your other patch are not appropriate this late in the -rc series, and Linus has ratched up his standards even higher with todays' RC release. You'll need to resubmit these for net-next. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2 net] 6lowpan: fix hop limit compression [not found] <1336656163-19382-1-git-send-email-y> 2012-05-10 13:22 ` [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check alex.bluesman.smirnov @ 2012-05-10 13:22 ` alex.bluesman.smirnov 1 sibling, 0 replies; 7+ messages in thread From: alex.bluesman.smirnov @ 2012-05-10 13:22 UTC (permalink / raw) To: davem; +Cc: netdev, Alexander Smirnov, Tony Cheneau From: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> Add missing pointer shift for the 'default' case. Signed-off-by: Alexander Smirnov <alex.bluesman.smirnov@gmail.com> Cc: Tony Cheneau <tony.cheneau+zigbeedev@amnesiak.org> --- net/ieee802154/6lowpan.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/net/ieee802154/6lowpan.c b/net/ieee802154/6lowpan.c index 0ab3efe..86f0013 100644 --- a/net/ieee802154/6lowpan.c +++ b/net/ieee802154/6lowpan.c @@ -492,6 +492,7 @@ static int lowpan_header_create(struct sk_buff *skb, break; default: *hc06_ptr = hdr->hop_limit; + hc06_ptr += 1; break; } -- 1.7.2.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-13 3:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1336656163-19382-1-git-send-email-y>
2012-05-10 13:22 ` [PATCH 1/2 net] 6lowpan: add missing pskb_may_pull() check alex.bluesman.smirnov
2012-05-10 13:30 ` Eric Dumazet
2012-05-10 14:05 ` Alexander Smirnov
2012-05-10 16:28 ` David Miller
2012-05-11 14:58 ` [PATCH 1/2 net v2] 6lowpan: rework data fetching from skb Alexander Smirnov
2012-05-13 3:28 ` David Miller
2012-05-10 13:22 ` [PATCH 2/2 net] 6lowpan: fix hop limit compression alex.bluesman.smirnov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.