Re: xfrm_input() and ->seq oddities

On Sat, Feb 02, 2008 at 11:58:27PM +0000, Al Viro wrote:

	What's going on with XFRM_SKB_CB(skb)->seq?  Almost all users
expect it to be u32 (feeding it to htonl()), except xfrm_input().
That one expects __be32 in there - check what it does to seq and
you'll see.  Moreover, it updates the sucker in a way that makes
sense only for __be32.

My bad.

	a) are we just missing ntohl() and htonl() in obvious places
in xfrm_input()?

It's semantically correct.  However, my mistake that I'm mixing the
input sequence number which is network endian with the output sequence
number which is host endian (and 64-bit).

	b) WTF is it declared as 64bit, anyway?

For extended sequence numbers which we'll support soon (I hope).

[IPSEC]: Fix bogus usage of u64 on input sequence number

Al Viro spotted a bogus use of u64 on the input sequence number which
is big-endian.  This patch fixes it by giving the input sequence number
its own member in the xfrm_skb_cb structure.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index ac72116..eea7785 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h

@@ -508,7 +508,10 @@ struct xfrm_skb_cb {
         } header;
 
         /* Sequence number for replay protection. */
-        u64 seq;
+	union {
+		u64 output;
+		__be32 input;
+	} seq;
 };
 
 #define XFRM_SKB_CB(__skb) ((struct xfrm_skb_cb *)&((__skb)->cb[0]))

diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 9d4555e..8219b7e 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c

@@ -96,7 +96,7 @@ static int ah_output(struct xfrm_state *x, struct sk_buff *skb)
 
 	ah->reserved = 0;
 	ah->spi = x->id.spi;
-	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
+	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output);
 
 	spin_lock_bh(&x->lock);
 	err = ah_mac_digest(ahp, skb, ah->auth_data);

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index 258d176..091e670 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c

@@ -199,7 +199,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	}
 
 	esph->spi = x->id.spi;
-	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
+	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output);
 
 	sg_init_table(sg, nfrags);
 	skb_to_sgvec(skb, sg,

@@ -210,7 +210,8 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb)
 	aead_givcrypt_set_callback(req, 0, esp_output_done, skb);
 	aead_givcrypt_set_crypt(req, sg, sg, clen, iv);
 	aead_givcrypt_set_assoc(req, asg, sizeof(*esph));
-	aead_givcrypt_set_giv(req, esph->enc_data, XFRM_SKB_CB(skb)->seq);
+	aead_givcrypt_set_giv(req, esph->enc_data,
+			      XFRM_SKB_CB(skb)->seq.output);
 
 	ESP_SKB_CB(skb)->tmp = tmp;
 	err = crypto_aead_givencrypt(req);

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 379c8e0..2ff0c82 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c

@@ -283,7 +283,7 @@ static int ah6_output(struct xfrm_state *x, struct sk_buff *skb)
 
 	ah->reserved = 0;
 	ah->spi = x->id.spi;
-	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
+	ah->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output);
 
 	spin_lock_bh(&x->lock);
 	err = ah_mac_digest(ahp, skb, ah->auth_data);

diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 8e0f142..0ec1402 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c

@@ -188,7 +188,7 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	*skb_mac_header(skb) = IPPROTO_ESP;
 
 	esph->spi = x->id.spi;
-	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
+	esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output);
 
 	sg_init_table(sg, nfrags);
 	skb_to_sgvec(skb, sg,

@@ -199,7 +199,8 @@ static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
 	aead_givcrypt_set_callback(req, 0, esp_output_done, skb);
 	aead_givcrypt_set_crypt(req, sg, sg, clen, iv);
 	aead_givcrypt_set_assoc(req, asg, sizeof(*esph));
-	aead_givcrypt_set_giv(req, esph->enc_data, XFRM_SKB_CB(skb)->seq);
+	aead_givcrypt_set_giv(req, esph->enc_data,
+			      XFRM_SKB_CB(skb)->seq.output);
 
 	ESP_SKB_CB(skb)->tmp = tmp;
 	err = crypto_aead_givencrypt(req);

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index 4d6ebc6..62188c6 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c

@@ -109,7 +109,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	if (encap_type < 0) {
 		async = 1;
 		x = xfrm_input_state(skb);
-		seq = XFRM_SKB_CB(skb)->seq;
+		seq = XFRM_SKB_CB(skb)->seq.input;
 		goto resume;
 	}
 

@@ -175,7 +175,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		spin_unlock(&x->lock);
 
-		XFRM_SKB_CB(skb)->seq = seq;
+		XFRM_SKB_CB(skb)->seq.input = seq;
 
 		nexthdr = x->type->input(x, skb);
 

diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index fc69036..569d377 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c

@@ -62,7 +62,7 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
 		}
 
 		if (x->type->flags & XFRM_TYPE_REPLAY_PROT) {
-			XFRM_SKB_CB(skb)->seq = ++x->replay.oseq;
+			XFRM_SKB_CB(skb)->seq.output = ++x->replay.oseq;
 			if (unlikely(x->replay.oseq == 0)) {
 				XFRM_INC_STATS(LINUX_MIB_XFRMOUTSTATESEQERROR);
 				x->replay.oseq--;

On Sun, Feb 03, 2008 at 11:20:19AM +1100, Herbert Xu wrote:

Al Viro spotted a bogus use of u64 on the input sequence number which
is big-endian.  This patch fixes it by giving the input sequence number
its own member in the xfrm_skb_cb structure.

This is still very odd...  Where do you initialize ->seq.input?  What
guarantees that async call of xfrm_input() will be always preceded by
at least one non-async one?

On Sun, Feb 03, 2008 at 12:37:19AM +0000, Al Viro wrote:

This is still very odd...  Where do you initialize ->seq.input?  What

In xfrm_input.

guarantees that async call of xfrm_input() will be always preceded by
at least one non-async one?

OK I admit it isn't pretty.  But the encap_type argument is reused to
indicate async resumption.  That is, if we enter with encap_type < 0,
it means that we're resuming a previous operation and seq.input has
therefore been set by the previous xfrm_input call.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Sun, Feb 03, 2008 at 02:05:16PM +1100, Herbert Xu wrote:

On Sun, Feb 03, 2008 at 12:37:19AM +0000, Al Viro wrote:

quoted

This is still very odd...  Where do you initialize ->seq.input?  What

In xfrm_input.

quoted

guarantees that async call of xfrm_input() will be always preceded by
at least one non-async one?

OK I admit it isn't pretty.  But the encap_type argument is reused to
indicate async resumption.  That is, if we enter with encap_type < 0,
it means that we're resuming a previous operation and seq.input has
therefore been set by the previous xfrm_input call.

*Ouch*

So what you are saying is
	* callers of xfrm_input_resume() are in callbacks that couldn't
have been set other than from esp_input()/esp6_input()
	* these two could have only been called via ->type->input()
	* ->type->input() is called from xfrm_input(), immediately after
having set ->seq.input, *or* from xfrm6_input_addr().  The former is safe.
	* xfrm6_input_addr() calls ->type->input() of object it gets from
xfrm_state_lookup_byaddr().  The protocol number passed to the latter comes
from xfrm6_input_addr() argument.
	* the protocol numbers given to xfrm6_input_addr() by its callers
are IPPROTO_DSTOPTS and IPPROTO_ROUTING resp; ->input() instances in their
xfrm_type do *not* set callbacks that could lead to xfrm_input_resume(),
so we are safe.

IMO that at least deserves a comment near xfrm_input()...
doe

On Sun, Feb 03, 2008 at 11:04:44AM +0000, Al Viro wrote:

So what you are saying is
	* callers of xfrm_input_resume() are in callbacks that couldn't
have been set other than from esp_input()/esp6_input()
	* these two could have only been called via ->type->input()
	* ->type->input() is called from xfrm_input(), immediately after
having set ->seq.input, *or* from xfrm6_input_addr().  The former is safe.
	* xfrm6_input_addr() calls ->type->input() of object it gets from
xfrm_state_lookup_byaddr().  The protocol number passed to the latter comes
from xfrm6_input_addr() argument.
	* the protocol numbers given to xfrm6_input_addr() by its callers
are IPPROTO_DSTOPTS and IPPROTO_ROUTING resp; ->input() instances in their
xfrm_type do *not* set callbacks that could lead to xfrm_input_resume(),
so we are safe.

This doesn't look so bad if you take out the xfrm6_input_addr call.
And you can't blame that one on me :)

The xfrm6_input_addr function is really a parallel universe which has
nothing to do with IPsec.  It's used by Mobile IPv6 just because it
happened to fit in the same schema.

In other words, IPsec transforms such as ESP cannot be called from
xfrm6_input_addr and as such async resumption never occurs with
xfrm6_input_addr.

IMO that at least deserves a comment near xfrm_input()...

Sure.  There is already a comment about encap_type < 0 in there, but
I think you'll probably be able to explain it much better than I can
looking in from the outside so if you have a patch... :)

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sun, 3 Feb 2008 11:20:19 +1100

[IPSEC]: Fix bogus usage of u64 on input sequence number

Al Viro spotted a bogus use of u64 on the input sequence number which
is big-endian.  This patch fixes it by giving the input sequence number
its own member in the xfrm_skb_cb structure.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks Herbert.

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 4 Feb 2008 09:00:27 +1100

On Sun, Feb 03, 2008 at 11:04:44AM +0000, Al Viro wrote:

quoted

IMO that at least deserves a comment near xfrm_input()...

Sure.  There is already a comment about encap_type < 0 in there, but
I think you'll probably be able to explain it much better than I can
looking in from the outside so if you have a patch... :)

Agreed on all counts ;-)