[RFC PATCH] xfrm: avoid to send/receive the exceeding hard lifetime data

[RFC PATCH] xfrm: avoid to send/receive the exceeding hard lifetime data

[roy.qing.li]

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++;
+
 		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);
-- 
1.7.5.4

Re: [RFC PATCH] xfrm: avoid to send/receive the exceeding hard lifetime data

[Steffen Klassert]

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.

Re: [RFC PATCH] xfrm: avoid to send/receive the exceeding hard lifetime data

[RongQing Li]

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.
>

Re: [RFC PATCH] xfrm: avoid to send/receive the exceeding hard lifetime data

[Steffen Klassert]

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.

Re: [RFC PATCH] xfrm: avoid to send/receive the exceeding hard lifetime data

[RongQing Li]

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