ESP trailer_len calculation

ESP trailer_len calculation

[David Miller]

Eric Dumazet and I recently were looking into a strange artifact in
ESP ->trailer_len calculations.

Eric was seeing values like "17" which looked strange.

He foudn that it's because of this line in esp4.c:esp_init_state()

	x->props.trailer_len = align + 1 + crypto_aead_authsize(esp->aead);

which comes from commit:

commit c5c2523893747f88a83376abad310c8ad13f7197
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Apr 9 11:47:18 2007 -0700

    [XFRM]: Optimize MTU calculation

which is based upon discussion threads:

http://marc.info/?l=linux-netdev&m=115468159401118&w=2

and

http://marc.info/?l=linux-netdev&m=117561805827241&w=2

Even more strange, in the orignal version of this patch the
calcaluation is actually:

	x->props.trailer_len = align - 1 + esp->auth.icv_trunc_len;

(ie. 'align - 1' instead of 'align + 1')

It seems that this "- 1 " or "+ 1" term can be completely eliminated,
unless there are some funny semantics wrt. the padding area of ESP.

Patrick and Herbert, what do you guys think?

Re: ESP trailer_len calculation

[Herbert Xu]

On Fri, Sep 24, 2010 at 02:40:44PM -0700, David Miller wrote:
> 
> Eric Dumazet and I recently were looking into a strange artifact in
> ESP ->trailer_len calculations.
> 
> Eric was seeing values like "17" which looked strange.
> 
> He foudn that it's because of this line in esp4.c:esp_init_state()
> 
> 	x->props.trailer_len = align + 1 + crypto_aead_authsize(esp->aead);
> 
> which comes from commit:
> 
> commit c5c2523893747f88a83376abad310c8ad13f7197
> Author: Patrick McHardy <kaber@trash.net>
> Date:   Mon Apr 9 11:47:18 2007 -0700
> 
>     [XFRM]: Optimize MTU calculation
> 
> which is based upon discussion threads:
> 
> http://marc.info/?l=linux-netdev&m=115468159401118&w=2
> 
> and
> 
> http://marc.info/?l=linux-netdev&m=117561805827241&w=2
> 
> Even more strange, in the orignal version of this patch the
> calcaluation is actually:
> 
> 	x->props.trailer_len = align - 1 + esp->auth.icv_trunc_len;
> 
> (ie. 'align - 1' instead of 'align + 1')
> 
> It seems that this "- 1 " or "+ 1" term can be completely eliminated,
> unless there are some funny semantics wrt. the padding area of ESP.
> 
> Patrick and Herbert, what do you guys think?

The number 17 does look very strange, however, after going through
the logic it does seem correct.

To calculate the minimum safe trailer length we need to consider
the worst-case scenario, and that is a packet where the payload
just happens to be one byte less than the cipher block size.

ESP always adds two bytes, then pads to at least the cipher
block size, follwed by the authentication value.  So in the
worst case we need to add

	2 + (blocksize - 1) + authlen	=
	blocksize + 1 + authlen

which is exactly what Patrick's patch does.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: ESP trailer_len calculation

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 25 Sep 2010 14:23:17 +0800

> The number 17 does look very strange, however, after going through
> the logic it does seem correct.
> 
> To calculate the minimum safe trailer length we need to consider
> the worst-case scenario, and that is a packet where the payload
> just happens to be one byte less than the cipher block size.
> 
> ESP always adds two bytes, then pads to at least the cipher
> block size, follwed by the authentication value.  So in the
> worst case we need to add
> 
> 	2 + (blocksize - 1) + authlen	=
> 	blocksize + 1 + authlen
> 
> which is exactly what Patrick's patch does.

Thanks for explaining this Herbert, but I have to admit it's a bit
disappointing :-)

So what we have is that headerlen is actually a function f() which
depends upon the payload length and the size of any IP options, rather
than a fixed value that can be computed based upon the cipher
blocksize, encap mode, and device MTU.

I guess if we really cared about this we could make headerlen a method
rather than a value, but I doubt it's that much of an issue.