| Message ID | 20180717152620.GB22664@Rafael-Mac.intra.sownet.nl (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Stefan Schmidt |
| Headers | show |
Hi, On Tue, Jul 17, 2018 at 05:26:20PM +0200, Rafael Vuijk wrote: > 6LoWPAN reassembly fragment overlap checks. > > Signed-off-by: Rafael Vuijk <r.vuijk@sownet.nl> > --- ./net/ieee802154/6lowpan/reassembly.c 2018-02-20 11:10:06.000000000 +0100 > +++ ./net/ieee802154/6lowpan/reassembly.c 2018-02-21 09:13:29.000000000 +0100 > @@ -179,6 +170,13 @@ static int lowpan_frag_queue(struct lowp > } > > found: > + /* Current fragment overlaps with previous fragment? */ > + if (prev && (lowpan_802154_cb(prev)->d_offset << 3) + prev->len > offset) > + goto err; > + /* Current fragment overlaps with next fragment? */ > + if (next && offset + skb->len > lowpan_802154_cb(next)->d_offset << 3) > + goto err; > + I have some thought of mine when seeing this code. The function is separated into two phases: phase 0: - finding the missing piece of fragment skb in the right order - if found goto found: phase 1: - everything after found to reassemble everything if we have everything together. This patch moves parts which are belongs to "phase 0" to "phase 1". I think the general idea in this algorithmn is to simple don't make checks on invalid things at all. If "broken" fragments are inside the "fragment bucket" then simple leave it there. There exists a garbage collector, controlled by some expire parameter to drop them then (60 seconds by default). Important is that the code should never run lowpan_frag_reasm() when something is not right in the fragments. - Alex -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 17.07.2018 17:26, Rafael Vuijk wrote: > 6LoWPAN reassembly fragment overlap checks. A bit more explanation would not hurd here. :-) > Signed-off-by: Rafael Vuijk <r.vuijk@sownet.nl> > --- ./net/ieee802154/6lowpan/reassembly.c 2018-02-20 11:10:06.000000000 +0100 > +++ ./net/ieee802154/6lowpan/reassembly.c 2018-02-21 09:13:29.000000000 +0100 > @@ -179,6 +170,13 @@ static int lowpan_frag_queue(struct lowp > } > > found: > + /* Current fragment overlaps with previous fragment? */ > + if (prev && (lowpan_802154_cb(prev)->d_offset << 3) + prev->len > offset)> + goto err; > + /* Current fragment overlaps with next fragment? */ > + if (next && offset + skb->len > lowpan_802154_cb(next)->d_offset << 3) To be honest, I am not sure I would get the operator priorities right in my head for this line without looking it up. :-) Could we get some () to help with the operator priorities? Even if that means we need to break this line into two. > + goto err; > + > /* Insert this fragment in the chain of fragments. */ > skb->next = next; > if (!next) regards Stefan Schmidt -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 18.07.2018 16:30, Alexander Aring wrote: > Hi, > > On Tue, Jul 17, 2018 at 05:26:20PM +0200, Rafael Vuijk wrote: >> 6LoWPAN reassembly fragment overlap checks. >> >> Signed-off-by: Rafael Vuijk <r.vuijk@sownet.nl> >> --- ./net/ieee802154/6lowpan/reassembly.c 2018-02-20 11:10:06.000000000 +0100 >> +++ ./net/ieee802154/6lowpan/reassembly.c 2018-02-21 09:13:29.000000000 +0100 >> @@ -179,6 +170,13 @@ static int lowpan_frag_queue(struct lowp >> } >> >> found: >> + /* Current fragment overlaps with previous fragment? */ >> + if (prev && (lowpan_802154_cb(prev)->d_offset << 3) + prev->len > offset) >> + goto err; >> + /* Current fragment overlaps with next fragment? */ >> + if (next && offset + skb->len > lowpan_802154_cb(next)->d_offset << 3) >> + goto err; >> + > > I have some thought of mine when seeing this code. The function is > separated into two phases: > > phase 0: > - finding the missing piece of fragment skb in the right order > - if found goto found: > > phase 1: > - everything after found to reassemble everything if we have everything > together. > > > This patch moves parts which are belongs to "phase 0" to "phase 1". I > think the general idea in this algorithmn is to simple don't make checks > on invalid things at all. If "broken" fragments are inside the "fragment > bucket" then simple leave it there. There exists a garbage collector, > controlled by some expire parameter to drop them then (60 seconds by > default). I think Rafael looked around for the best place of these checks. The algorithm is buggy in this regard as they have seen in their testing and packet traces. If you have a better place for this check I am pretty sure they would be happy to test it and come back to you if it fixes their issue. If we find no better place I will just use this patch and move forward to have at least the practical issue fixed. regards Stefan Schmidt -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- ./net/ieee802154/6lowpan/reassembly.c 2018-02-20 11:10:06.000000000 +0100 +++ ./net/ieee802154/6lowpan/reassembly.c 2018-02-21 09:13:29.000000000 +0100 @@ -179,6 +170,13 @@ static int lowpan_frag_queue(struct lowp } found: + /* Current fragment overlaps with previous fragment? */ + if (prev && (lowpan_802154_cb(prev)->d_offset << 3) + prev->len > offset) + goto err; + /* Current fragment overlaps with next fragment? */ + if (next && offset + skb->len > lowpan_802154_cb(next)->d_offset << 3) + goto err; + /* Insert this fragment in the chain of fragments. */ skb->next = next; if (!next)
6LoWPAN reassembly fragment overlap checks. Signed-off-by: Rafael Vuijk <r.vuijk@sownet.nl> -- To unsubscribe from this list: send the line "unsubscribe linux-wpan" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html