| Message ID | 1355387152-9963-1-git-send-email-roy.qing.li@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Dec 13, 2012 at 04:25:52PM +0800, roy.qing.li@gmail.com wrote: > From: Li RongQing <roy.qing.li@gmail.com> > > If setkey sets both bh and bs as 1024, and the total send and receive package > size is 1024, then if next package size is too large, this package should be > discard. > > Example, first package size is 1000, send success, then the second package > is 500, 1000+500 is larger than 1024, so the second package should be discard. > > Signed-off-by: Li RongQing <roy.qing.li@gmail.com> > --- > net/xfrm/xfrm_input.c | 6 +++--- > net/xfrm/xfrm_output.c | 6 +++--- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c > index ab2bb42..d0de8f3 100644 > --- a/net/xfrm/xfrm_input.c > +++ b/net/xfrm/xfrm_input.c > @@ -178,6 +178,9 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) > goto drop_unlock; > } > > + x->curlft.bytes += skb->len; > + x->curlft.packets++; > + This is a bit critical on input. We should only increment these values if the integrity check on this packet was successfull. Otherwise someone could spam us with invalid packets and trigger a state expiry. If a synchronous crypto algorithm is used, we send at most one packet too much. The maximal byte count was not yet reached and RFC 2401 says not much on how to handle the packet that reaches the maximal byte count, so this is probaply ok. But if an asynchronous crypto algorithm is used, we can send a lot of packets too much. So we should probaply add a second expiry check after resume from asynchronous crypto. We do this already with the replay check. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/12/13 Steffen Klassert <steffen.klassert@secunet.com>: > On Thu, Dec 13, 2012 at 04:25:52PM +0800, roy.qing.li@gmail.com wrote: >> From: Li RongQing <roy.qing.li@gmail.com> >> >> If setkey sets both bh and bs as 1024, and the total send and receive package >> size is 1024, then if next package size is too large, this package should be >> discard. >> >> Example, first package size is 1000, send success, then the second package >> is 500, 1000+500 is larger than 1024, so the second package should be discard. >> >> Signed-off-by: Li RongQing <roy.qing.li@gmail.com> >> --- >> net/xfrm/xfrm_input.c | 6 +++--- >> net/xfrm/xfrm_output.c | 6 +++--- >> 2 files changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c >> index ab2bb42..d0de8f3 100644 >> --- a/net/xfrm/xfrm_input.c >> +++ b/net/xfrm/xfrm_input.c >> @@ -178,6 +178,9 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) >> goto drop_unlock; >> } >> >> + x->curlft.bytes += skb->len; >> + x->curlft.packets++; >> + > > This is a bit critical on input. We should only increment these values > if the integrity check on this packet was successfull. Otherwise someone > could spam us with invalid packets and trigger a state expiry. > > If a synchronous crypto algorithm is used, we send at most one packet too > much. The maximal byte count was not yet reached and RFC 2401 says not > much on how to handle the packet that reaches the maximal byte count, > so this is probaply ok. > Yes, RFC does not say how to handle this packet. But when I do a IPsec compliance test with IxANVL, the test case 5.3/5.11, which reports a error because it expects this packet should be dropped, but not. I do not know if it is bug, or if it is valuable to fix it? -Li > But if an asynchronous crypto algorithm is used, we can send a lot > of packets too much. So we should probaply add a second expiry check > after resume from asynchronous crypto. We do this already with the replay > check. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 14, 2012 at 02:58:03PM +0800, RongQing Li wrote: > > Yes, RFC does not say how to handle this packet. > > But when I do a IPsec compliance test with IxANVL, the test case 5.3/5.11, > which reports a error because it expects this packet should be dropped, but not. > > > I do not know if it is bug, or if it is valuable to fix it? > As long as the RFC does not state anything else, we ar ok in the synchronous code path. But we need a fix for the asynchronous code path. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/12/14 Steffen Klassert <steffen.klassert@secunet.com>: > On Fri, Dec 14, 2012 at 02:58:03PM +0800, RongQing Li wrote: >> >> Yes, RFC does not say how to handle this packet. >> >> But when I do a IPsec compliance test with IxANVL, the test case 5.3/5.11, >> which reports a error because it expects this packet should be dropped, but not. >> >> >> I do not know if it is bug, or if it is valuable to fix it? >> > > As long as the RFC does not state anything else, we ar ok in the > synchronous code path. But we need a fix for the asynchronous > code path. > Thanks. -Roy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index ab2bb42..d0de8f3 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -178,6 +178,9 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) goto drop_unlock; } + x->curlft.bytes += skb->len; + x->curlft.packets++; + if (xfrm_state_check_expire(x)) { XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEEXPIRED); goto drop_unlock; @@ -219,9 +222,6 @@ resume: x->repl->advance(x, seq); - x->curlft.bytes += skb->len; - x->curlft.packets++; - spin_unlock(&x->lock); XFRM_MODE_SKB_CB(skb)->protocol = nexthdr; diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c index 95a338c..0f38cb2 100644 --- a/net/xfrm/xfrm_output.c +++ b/net/xfrm/xfrm_output.c @@ -61,6 +61,9 @@ static int xfrm_output_one(struct sk_buff *skb, int err) } spin_lock_bh(&x->lock); + + x->curlft.bytes += skb->len; + x->curlft.packets++; err = xfrm_state_check_expire(x); if (err) { XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTSTATEEXPIRED); @@ -73,9 +76,6 @@ static int xfrm_output_one(struct sk_buff *skb, int err) goto error; } - x->curlft.bytes += skb->len; - x->curlft.packets++; - spin_unlock_bh(&x->lock); skb_dst_force(skb);