[PATCHv2 net-next 0/3] IPComp fixes

[PATCHv2 net-next 0/3] IPComp fixes

[Fan Du]

Hi,

This patchset includes bug fix focusing on IPComp mainly, which addresses
IPComp Compression Parameter Index(CPI, 16bits of 32bits SPI value) problem.

And other issue is the compression part, undesirable packet will not be
compressed, and then send into wire, this packet will be dropped by policy
checking in the receiver side.

v2:
  Patch1: Clean up align issue from v1.
  Patch2: Introduce verify_userspi_info for pkfey and netlink interface
          to share code.
  Patch3: Add Documentation/networking/ipsec.txt to document known issues. 

Fan Du (3):
  xfrm: check user specified spi for IPComp
  xfrm: export verify_userspi_info for pkfey and netlink interface
  xfrm: Add file to document IPsec corner case

 Documentation/networking/ipsec.txt |   40 ++++++++++++++++++++++++++++++++++++
 include/net/xfrm.h                 |    1 +
 net/key/af_key.c                   |    6 ++++++
 net/xfrm/xfrm_state.c              |   24 ++++++++++++++++++++++
 net/xfrm/xfrm_user.c               |   29 ++++----------------------
 5 files changed, 75 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/networking/ipsec.txt

-- 
1.7.9.5

[PATCHv2 net-next 1/3] xfrm: check user specified spi for IPComp

[Fan Du]

IPComp connection between two hosts is broken if given spi bigger
than 0xffff.

OUTSPI=0x87
INSPI=0x11112

ip xfrm policy update dst 192.168.1.101 src 192.168.1.109 dir out action allow \
       tmpl dst 192.168.1.101 src 192.168.1.109 proto comp spi $OUTSPI
ip xfrm policy update src 192.168.1.101 dst 192.168.1.109 dir in action allow \
       tmpl src 192.168.1.101 dst 192.168.1.109 proto comp spi $INSPI

ip xfrm state add src 192.168.1.101 dst 192.168.1.109  proto comp spi $INSPI \
		comp deflate
ip xfrm state add dst 192.168.1.101 src 192.168.1.109  proto comp spi $OUTSPI \
		comp deflate

tcpdump can capture outbound ping packet, but inbound packet is
dropped with XfrmOutNoStates errors. It looks like spi value used
for IPComp is expected to be 16bits wide only.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/xfrm/xfrm_user.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index f964d4c..8543b1b 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -181,7 +181,9 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 		    attrs[XFRMA_ALG_AEAD]	||
 		    attrs[XFRMA_ALG_CRYPT]	||
 		    attrs[XFRMA_ALG_COMP]	||
-		    attrs[XFRMA_TFCPAD])
+		    attrs[XFRMA_TFCPAD]		||
+		    (ntohl(p->id.spi) >= 0x10000))
+
 			goto out;
 		break;
 
-- 
1.7.9.5

[PATCHv2 net-next 2/3] xfrm: export verify_userspi_info for pkfey and netlink interface

[Fan Du]

In order to check against valid IPcomp spi range, export verify_userspi_info
for both pfkey and netlink interface.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 include/net/xfrm.h    |    1 +
 net/key/af_key.c      |    6 ++++++
 net/xfrm/xfrm_state.c |   24 ++++++++++++++++++++++++
 net/xfrm/xfrm_user.c  |   25 +------------------------
 4 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 6b82fdf..369fa99 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1564,6 +1564,7 @@ struct xfrm_policy *xfrm_policy_byid(struct net *net, u32 mark, u8, int dir,
 				     u32 id, int delete, int *err);
 int xfrm_policy_flush(struct net *net, u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
+int verify_spi_info(u8 proto, u32 min, u32 max);
 int xfrm_alloc_spi(struct xfrm_state *x, u32 minspi, u32 maxspi);
 struct xfrm_state *xfrm_find_acq(struct net *net, const struct xfrm_mark *mark,
 				 u8 mode, u32 reqid, u8 proto,
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 545f047..7605d51 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1340,6 +1340,12 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, const struct sadb_
 		max_spi = range->sadb_spirange_max;
 	}
 
+	err = verify_spi_info(x->id.proto, min_spi, max_spi);
+	if (err) {
+		xfrm_state_put(x);
+		return err; 
+	}
+
 	err = xfrm_alloc_spi(x, min_spi, max_spi);
 	resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x);
 
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 68c2f35..d953639 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1496,6 +1496,30 @@ u32 xfrm_get_acqseq(void)
 }
 EXPORT_SYMBOL(xfrm_get_acqseq);
 
+int verify_spi_info(u8 proto, u32 min, u32 max)
+{
+        switch (proto) {
+        case IPPROTO_AH:
+        case IPPROTO_ESP:
+                break;
+
+        case IPPROTO_COMP:
+                /* IPCOMP spi is 16-bits. */
+                if (max >= 0x10000)
+                        return -EINVAL;
+                break;
+
+        default:
+                return -EINVAL;
+        }    
+
+        if (min > max)
+                return -EINVAL;
+
+        return 0;
+}
+EXPORT_SYMBOL(verify_spi_info);
+
 int xfrm_alloc_spi(struct xfrm_state *x, u32 low, u32 high)
 {
 	struct net *net = xs_net(x);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8543b1b..f837983 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1076,29 +1076,6 @@ out_noput:
 	return err;
 }
 
-static int verify_userspi_info(struct xfrm_userspi_info *p)
-{
-	switch (p->info.id.proto) {
-	case IPPROTO_AH:
-	case IPPROTO_ESP:
-		break;
-
-	case IPPROTO_COMP:
-		/* IPCOMP spi is 16-bits. */
-		if (p->max >= 0x10000)
-			return -EINVAL;
-		break;
-
-	default:
-		return -EINVAL;
-	}
-
-	if (p->min > p->max)
-		return -EINVAL;
-
-	return 0;
-}
-
 static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 		struct nlattr **attrs)
 {
@@ -1113,7 +1090,7 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct xfrm_mark m;
 
 	p = nlmsg_data(nlh);
-	err = verify_userspi_info(p);
+	err = verify_spi_info(p->info.id.proto, p->min, p->max);
 	if (err)
 		goto out_noput;
 
-- 
1.7.9.5

Re: [PATCHv2 net-next 2/3] xfrm: export verify_userspi_info for pkfey and netlink interface

[Steffen Klassert]

On Sun, Dec 15, 2013 at 05:19:53PM +0800, Fan Du wrote:
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 545f047..7605d51 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -1340,6 +1340,12 @@ static int pfkey_getspi(struct sock *sk, struct sk_buff *skb, const struct sadb_
>  		max_spi = range->sadb_spirange_max;
>  	}
>  
> +	err = verify_spi_info(x->id.proto, min_spi, max_spi);
> +	if (err) {
> +		xfrm_state_put(x);
> +		return err; 

This line adds a trailing whitespace.

> +	}
> +
>  	err = xfrm_alloc_spi(x, min_spi, max_spi);
>  	resp_skb = err ? ERR_PTR(err) : pfkey_xfrm_state2msg(x);
>  
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 68c2f35..d953639 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1496,6 +1496,30 @@ u32 xfrm_get_acqseq(void)
>  }
>  EXPORT_SYMBOL(xfrm_get_acqseq);
>  
> +int verify_spi_info(u8 proto, u32 min, u32 max)
> +{
> +        switch (proto) {
> +        case IPPROTO_AH:
> +        case IPPROTO_ESP:
> +                break;
> +
> +        case IPPROTO_COMP:
> +                /* IPCOMP spi is 16-bits. */
> +                if (max >= 0x10000)
> +                        return -EINVAL;
> +                break;
> +
> +        default:
> +                return -EINVAL;
> +        }    

This one too.

[PATCHv2 net-next 3/3] xfrm: Add file to document IPsec corner case

[Fan Du]

Create Documentation/networking/ipsec.txt to document IPsec
corner issues and other info, which will be useful when user
deploying IPsec.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 Documentation/networking/ipsec.txt |   40 ++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/networking/ipsec.txt

diff --git a/Documentation/networking/ipsec.txt b/Documentation/networking/ipsec.txt
new file mode 100644
index 0000000..3b02806
--- /dev/null
+++ b/Documentation/networking/ipsec.txt
@@ -0,0 +1,40 @@
+
+Here documents known IPsec corner cases which need to be keep in mind when
+deploy various IPsec configuration in real world production environment.
+
+1. IPcomp: Small IP packet won't get compressed at sender, and failed on
+	   policy check on receiver.
+
+Quote from RFC3173:
+2.2. Non-Expansion Policy
+
+   If the total size of a compressed payload and the IPComp header, as
+   defined in section 3, is not smaller than the size of the original
+   payload, the IP datagram MUST be sent in the original non-compressed
+   form.  To clarify: If an IP datagram is sent non-compressed, no
+
+   IPComp header is added to the datagram.  This policy ensures saving
+   the decompression processing cycles and avoiding incurring IP
+   datagram fragmentation when the expanded datagram is larger than the
+   MTU.
+
+   Small IP datagrams are likely to expand as a result of compression.
+   Therefore, a numeric threshold should be applied before compression,
+   where IP datagrams of size smaller than the threshold are sent in the
+   original form without attempting compression.  The numeric threshold
+   is implementation dependent.
+
+Current IPComp implementation is indeed by the book, while as in practice
+when sending non-compressed packet to the peer(whether or not packet len
+is smaller than the threshold or the compressed len is large than original
+packet len), the packet is dropped when checking the policy as this packet
+matches the selector but not coming from any XFRM layer, i.e., with no
+security path. Such naked packet will not eventually make it to upper layer.
+The result is much more wired to the user when ping peer with different
+payload length.
+
+One workaround is try to set "level use" for each policy if user observed
+above scenario. The consequence of doing so is small packet(uncompressed)
+will skip policy checking on receiver side.
+
+
-- 
1.7.9.5

Re: [PATCHv2 net-next 3/3] xfrm: Add file to document IPsec corner case

[Steffen Klassert]

On Sun, Dec 15, 2013 at 05:19:54PM +0800, Fan Du wrote:
> Create Documentation/networking/ipsec.txt to document IPsec
> corner issues and other info, which will be useful when user
> deploying IPsec.
> 
> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
>  Documentation/networking/ipsec.txt |   40 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/networking/ipsec.txt
> 
> diff --git a/Documentation/networking/ipsec.txt b/Documentation/networking/ipsec.txt
> new file mode 100644
> index 0000000..3b02806
> --- /dev/null
> +++ b/Documentation/networking/ipsec.txt
> @@ -0,0 +1,40 @@
> +
> +Here documents known IPsec corner cases which need to be keep in mind when
> +deploy various IPsec configuration in real world production environment.
> +
> +1. IPcomp: Small IP packet won't get compressed at sender, and failed on
> +	   policy check on receiver.
> +
> +Quote from RFC3173:
> +2.2. Non-Expansion Policy
> +
> +   If the total size of a compressed payload and the IPComp header, as
> +   defined in section 3, is not smaller than the size of the original
> +   payload, the IP datagram MUST be sent in the original non-compressed
> +   form.  To clarify: If an IP datagram is sent non-compressed, no
> +
> +   IPComp header is added to the datagram.  This policy ensures saving
> +   the decompression processing cycles and avoiding incurring IP
> +   datagram fragmentation when the expanded datagram is larger than the
> +   MTU.
> +
> +   Small IP datagrams are likely to expand as a result of compression.
> +   Therefore, a numeric threshold should be applied before compression,
> +   where IP datagrams of size smaller than the threshold are sent in the
> +   original form without attempting compression.  The numeric threshold
> +   is implementation dependent.
> +
> +Current IPComp implementation is indeed by the book, while as in practice
> +when sending non-compressed packet to the peer(whether or not packet len
> +is smaller than the threshold or the compressed len is large than original
> +packet len), the packet is dropped when checking the policy as this packet
> +matches the selector but not coming from any XFRM layer, i.e., with no
> +security path. Such naked packet will not eventually make it to upper layer.
> +The result is much more wired to the user when ping peer with different
> +payload length.
> +
> +One workaround is try to set "level use" for each policy if user observed
> +above scenario. The consequence of doing so is small packet(uncompressed)
> +will skip policy checking on receiver side.
> +
> +

Please remove the empty lines at the end of the file.

Also, it might be good to mention what the user exactly
has configure do to get a workaround.

Re: [PATCHv2 net-next 3/3] xfrm: Add file to document IPsec corner case

[Fan Du]

On 2013年12月16日 17:46, Steffen Klassert wrote:
> On Sun, Dec 15, 2013 at 05:19:54PM +0800, Fan Du wrote:
>> Create Documentation/networking/ipsec.txt to document IPsec
>> corner issues and other info, which will be useful when user
>> deploying IPsec.
>>
>> Signed-off-by: Fan Du<fan.du@windriver.com>
>> ---
>>   Documentation/networking/ipsec.txt |   40 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/networking/ipsec.txt
>>
>> diff --git a/Documentation/networking/ipsec.txt b/Documentation/networking/ipsec.txt
>> new file mode 100644
>> index 0000000..3b02806
>> --- /dev/null
>> +++ b/Documentation/networking/ipsec.txt
>> @@ -0,0 +1,40 @@
>> +
>> +Here documents known IPsec corner cases which need to be keep in mind when
>> +deploy various IPsec configuration in real world production environment.
>> +
>> +1. IPcomp: Small IP packet won't get compressed at sender, and failed on
>> +	   policy check on receiver.
>> +
>> +Quote from RFC3173:
>> +2.2. Non-Expansion Policy
>> +
>> +   If the total size of a compressed payload and the IPComp header, as
>> +   defined in section 3, is not smaller than the size of the original
>> +   payload, the IP datagram MUST be sent in the original non-compressed
>> +   form.  To clarify: If an IP datagram is sent non-compressed, no
>> +
>> +   IPComp header is added to the datagram.  This policy ensures saving
>> +   the decompression processing cycles and avoiding incurring IP
>> +   datagram fragmentation when the expanded datagram is larger than the
>> +   MTU.
>> +
>> +   Small IP datagrams are likely to expand as a result of compression.
>> +   Therefore, a numeric threshold should be applied before compression,
>> +   where IP datagrams of size smaller than the threshold are sent in the
>> +   original form without attempting compression.  The numeric threshold
>> +   is implementation dependent.
>> +
>> +Current IPComp implementation is indeed by the book, while as in practice
>> +when sending non-compressed packet to the peer(whether or not packet len
>> +is smaller than the threshold or the compressed len is large than original
>> +packet len), the packet is dropped when checking the policy as this packet
>> +matches the selector but not coming from any XFRM layer, i.e., with no
>> +security path. Such naked packet will not eventually make it to upper layer.
>> +The result is much more wired to the user when ping peer with different
>> +payload length.
>> +
>> +One workaround is try to set "level use" for each policy if user observed
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here

>> +above scenario. The consequence of doing so is small packet(uncompressed)
>> +will skip policy checking on receiver side.
>> +
>> +
>
> Please remove the empty lines at the end of the file.
>
> Also, it might be good to mention what the user exactly
> has configure do to get a workaround.
     It's in above here..

Will fix while space error, sorry for such mistakes.

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv2 net-next 3/3] xfrm: Add file to document IPsec corner case

[Steffen Klassert]

On Mon, Dec 16, 2013 at 05:58:50PM +0800, Fan Du wrote:
> 
> 
> On 2013年12月16日 17:46, Steffen Klassert wrote:
> >On Sun, Dec 15, 2013 at 05:19:54PM +0800, Fan Du wrote:
> >>+
> >>+One workaround is try to set "level use" for each policy if user observed
>                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ here
> 
> >>+above scenario. The consequence of doing so is small packet(uncompressed)
> >>+will skip policy checking on receiver side.
> >>+
> >>+
> >
> >Please remove the empty lines at the end of the file.
> >
> >Also, it might be good to mention what the user exactly
> >has configure do to get a workaround.
>     It's in above here..
> 

Ok.