Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[David Dillow]

[Apologies for the dupe, fixing stupid typo for netdev address...
-ENOCAFFEINE]

I hesitate to reward the squeaky wheel, but in the community spirit,
here goes...

Please fix the subject for future submissions. The subject should be a
short, one line description of the patch. It helps if the subject
includes the section of the code you are affecting. Also, if you are
resending a patch after addressing review comments, change the tag to
[PATCH v2] etc. will indicate that. For example:
	[PATCH v2] crypto: add support for the NIST CMAC hash

This keeps the maintainers from having to hand edit your patch, which
dramatically slows them down. The goal is to get to a patch that can be
applied as-is after review.

On Mon, 2013-01-21 at 07:57 -0500, Tom St Denis wrote:
> Hey all,
> 
> Here's an updated patch which addresses a couple of build issues and
> coding style complaints.  
> 
> I still can't get it to run via testmgr I get 
> 
> [  162.407807] alg: No test for cmac(aes) (cmac(aes-generic))
> 
> Despite the fact I have an entry for cmac(aes) (much like xcbc(aes)...).
> 
> Here's the patch to bring 3.8-rc4 up with CMAC ...

All of this commentary should go after the '---' separator; the
maintainer will have to hand edit it out otherwise. It's good
information, it's just the wrong place.

There should be a short description here of the patch, if needed. You
may be fine with the one line description in this case, or you could
point to the RFC, etc.

> Signed-off-by: Tom St Denis <tstdenis@elliptictech.com>
> 
> ---
>  crypto/Kconfig               |   8 ++
>  crypto/Makefile              |   1 +
>  crypto/cmac.c                | 317 +++++++++++++++++++++++++++++++++++++++++++
>  crypto/testmgr.c             |   9 ++
>  crypto/testmgr.h             |  52 +++++++
>  include/uapi/linux/pfkeyv2.h |   1 +
>  net/xfrm/xfrm_algo.c         |  17 +++

You may be asked to split out the net changes into a separate patch, but
since you are adding the user at the time you add the code, you may not.

>  7 files changed, 405 insertions(+)
>  create mode 100644 crypto/cmac.c
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 4641d95..5ac2c7f 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -301,6 +301,14 @@ config CRYPTO_XCBC
>  		http://csrc.nist.gov/encryption/modes/proposedmodes/
>  		 xcbc-mac/xcbc-mac-spec.pdf
>  
> +config CRYPTO_CMAC
> +	tristate "CMAC support"
> +	depends on EXPERIMENTAL
> +	select CRYPTO_HASH
> +	select CRYPTO_MANAGER
> +	help
> +	  NIST CMAC cipher based MAC

Pointers to the docs as for XCBC above would be useful here.

> diff --git a/crypto/cmac.c b/crypto/cmac.c
> new file mode 100644
> index 0000000..1ffeea7
> --- /dev/null
> +++ b/crypto/cmac.c
> @@ -0,0 +1,317 @@
> +/*
> + * Copyright (C)2006 USAGI/WIDE Project

Add your copyright info, 2013?

> +static int crypto_cmac_digest_setkey(struct crypto_shash *parent,
> +					  const u8 *inkey, unsigned int keylen)
> +{
> +	unsigned long alignmask = crypto_shash_alignmask(parent);
> +	struct cmac_tfm_ctx *ctx = crypto_shash_ctx(parent);
> +	int bs = crypto_shash_blocksize(parent);
> +	u8 *consts = PTR_ALIGN(&ctx->ctx[0], alignmask + 1);
> +	int x, y, err = 0;
> +	u8 msb, mask;
> +
> +	switch (bs) {
> +	case 16:
> +		mask = 0x87;
> +		break;
> +	case 8:
> +		mask = 0x1B;
> +		break;
> +	default:
> +		return -EINVAL; /*  only support 64/128 bit block ciphers */

Checkpatch doesn't warn, but this comment would probably be preferred to
be on a line by itself.

> +	for (x = 0; x < 2; x++) {
> +		/* if msb(L * u^(x+1)) = 0 then just shift,
> +		otherwise shift and xor constant mask */

This comment is incorrectly formatted; I see checkpatch.pl from 3.7-rc4
didn't pick it up, which is interesting.
		/*
		 * if msb(L * u^(x+1)) = 0 then just shift,
		 * otherwise shift and xor constant mask
		 */

Though I'll note that doing 
	grep '/\*' crypto/*.c | grep -v '\*/' | less

provides evidence that it also commonly
		/* is msb....
		 */

> +		/* shift left */
> +		for (y = 0; y < (bs - 1); y++)
> +			consts[x*bs + y] =
> +				((consts[x*bs + y] << 1) |
> +				(consts[x*bs + y+1] >> 7)) & 255;

So, here is a case where you fixed two warnings at the same time, but
made one of them irrelevant in the process -- this should have braces
around the single statement. checkpatch.pl complained initially because
there was a one-line statement, but would not have complained if it
looked like you have it now. Also, probably want a spaces in the y+1, if
not the multiplication.

> +
> +		consts[x*bs + bs - 1] =
> +			((consts[x*bs + bs - 1] << 1) ^
> +			(msb ? mask : 0)) & 255;
> +
> +		/* copy up as require */

Minor English nit: required?

> +		if (x == 0)
> +			memcpy(&consts[(x+1)*bs], &consts[x*bs], bs);

perhaps some spacing, though I'm personally OK with it.

> diff --git a/crypto/testmgr.h b/crypto/testmgr.h
> index b5721e0..9688bfe 100644
> --- a/crypto/testmgr.h
> +++ b/crypto/testmgr.h
> @@ -1639,6 +1639,58 @@ static struct hash_testvec hmac_sha256_tv_template[] = {
>  	},
>  };
>  
> +#define CMAC_AES_TEST_VECTORS 4
> +
> +static struct hash_testvec aes_cmac128_tv_template[] = {
> +	{
> +		.key  = "\x2b\x7e\x15\x16\x28\xae\xd2\xa6\xab"
> +		"\xf7\x15\x88\x09\xcf\x4f\x3c",

There does seem to be some inconsistencies in the test vector
formatting, but it seems to be pretty common for the hex dumps to put 8
bytes in each part of the string, and to most line of the strings up.

Doing this alignment is how I found the missing \x in your first
submission.

> +		.plaintext = zeroed_string,
> +		.digest = "\xbb\x1d\x69\x29\xe9\x59\x37\x28\x7f"
> +		"\xa3\x7d\x12\x9b\x75\x67\x46",
> +		.psize   = 0,
> +		.ksize   = 16,
> +	},
> +
> +	{

The rest of the file uses "}, { " between test vectors.


Because you are working in the networking code, you should cc netdev --
they need to review the following hunks. I don't know what the
allocation policy is for adding algorithm numbers in pfkeyv2, but they
would know if you are creating a conflict with other work.

> diff --git a/include/uapi/linux/pfkeyv2.h b/include/uapi/linux/pfkeyv2.h
> index 0b80c80..d61898e 100644
> --- a/include/uapi/linux/pfkeyv2.h
> +++ b/include/uapi/linux/pfkeyv2.h
> @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
>  #define SADB_X_AALG_SHA2_512HMAC	7
>  #define SADB_X_AALG_RIPEMD160HMAC	8
>  #define SADB_X_AALG_AES_XCBC_MAC	9
> +#define SADB_X_AALG_AES_CMAC_MAC	10
>  #define SADB_X_AALG_NULL		251	/* kame */
>  #define SADB_AALG_MAX			251
>  
> diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
> index 4ce2d93..bd6f227 100644
> --- a/net/xfrm/xfrm_algo.c
> +++ b/net/xfrm/xfrm_algo.c
> @@ -265,6 +265,23 @@ static struct xfrm_algo_desc aalg_list[] = {
>  	}
>  },
>  {
> +	.name = "cmac(aes)",
> +
> +	.uinfo = {
> +		.auth = {
> +			.icv_truncbits = 96,
> +			.icv_fullbits = 128,
> +		}
> +	},
> +
> +	.desc = {
> +		.sadb_alg_id = SADB_X_AALG_AES_CMAC_MAC,
> +		.sadb_alg_ivlen = 0,
> +		.sadb_alg_minbits = 128,
> +		.sadb_alg_maxbits = 128
> +	}
> +},
> +{
>  	.name = "xcbc(aes)",
>  
>  	.uinfo = {

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

(Cc netdev@vger.kernel.org)

On Mon, Jan 21, 2013 at 07:57:28AM -0500, Tom St Denis wrote:
> Hey all,
> 
> Here's an updated patch which addresses a couple of build issues and coding style complaints.  
> 
> I still can't get it to run via testmgr I get 
> 
> [  162.407807] alg: No test for cmac(aes) (cmac(aes-generic))
> 
> Despite the fact I have an entry for cmac(aes) (much like xcbc(aes)...).
> 
> Here's the patch to bring 3.8-rc4 up with CMAC ...
> 

As already pointed out by David Dillow, the above comments should go
after the '---' separator. Please use the subject line and commit
message from the previous version of this patch when you resend,
this was ok.

> Signed-off-by: Tom St Denis <tstdenis@elliptictech.com>
> 
> ---
>  crypto/Kconfig               |   8 ++
>  crypto/Makefile              |   1 +
>  crypto/cmac.c                | 317 +++++++++++++++++++++++++++++++++++++++++++
>  crypto/testmgr.c             |   9 ++
>  crypto/testmgr.h             |  52 +++++++
>  include/uapi/linux/pfkeyv2.h |   1 +
>  net/xfrm/xfrm_algo.c         |  17 +++
>  7 files changed, 405 insertions(+)
>  create mode 100644 crypto/cmac.c
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index 4641d95..5ac2c7f 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -301,6 +301,14 @@ config CRYPTO_XCBC
>  		http://csrc.nist.gov/encryption/modes/proposedmodes/
>  		 xcbc-mac/xcbc-mac-spec.pdf
>  
> +config CRYPTO_CMAC
> +	tristate "CMAC support"
> +	depends on EXPERIMENTAL

CONFIG_EXPERIMENTAL is going to be removed in linux-next,
please remove this dependency.

> +	select CRYPTO_HASH
> +	select CRYPTO_MANAGER
> +	help
> +	  NIST CMAC cipher based MAC
> +

Please add some more information to the help text. Everybody who knows
what a 'NIST CMAC cipher based MAC' is will not need this help text, all
the others are left with almost no information. This help text is for
users in the first place, not for developers. So it should contain some
informations about this cipher and/or some pointers where to find
informations.

Please Cc me on your next version because we plan to route your
patch via the ipsec-next tree into the mainline.

Thanks!

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Jussi Kivilinna]

Quoting Tom St Denis <tstdenis@elliptictech.com>:

> Hey all,
>
> Here's an updated patch which addresses a couple of build issues and  
> coding style complaints.
>
> I still can't get it to run via testmgr I get
>
> [  162.407807] alg: No test for cmac(aes) (cmac(aes-generic))
>
> Despite the fact I have an entry for cmac(aes) (much like xcbc(aes)...).
>
> Here's the patch to bring 3.8-rc4 up with CMAC ...
>
> Signed-off-by: Tom St Denis <tstdenis@elliptictech.com>
>
<snip>
> diff --git a/include/uapi/linux/pfkeyv2.h b/include/uapi/linux/pfkeyv2.h
> index 0b80c80..d61898e 100644
> --- a/include/uapi/linux/pfkeyv2.h
> +++ b/include/uapi/linux/pfkeyv2.h
> @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
>  #define SADB_X_AALG_SHA2_512HMAC	7
>  #define SADB_X_AALG_RIPEMD160HMAC	8
>  #define SADB_X_AALG_AES_XCBC_MAC	9
> +#define SADB_X_AALG_AES_CMAC_MAC	10
>  #define SADB_X_AALG_NULL		251	/* kame */
>  #define SADB_AALG_MAX			251

Should these values be based on IANA assigned IPSEC AH transform identifiers?

https://www.iana.org/assignments/isakmp-registry/isakmp-registry.xml#isakmp-registry-6

-Jussi

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Tom St Denis]

----- Original Message -----
> From: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: linux-kernel@vger.kernel.org, "Herbert Xu" <herbert@gondor.apana.org.au>, "David Miller" <davem@davemloft.net>,
> linux-crypto@vger.kernel.org, "Steffen Klassert" <steffen.klassert@secunet.com>, netdev@vger.kernel.org
> Sent: Wednesday, 23 January, 2013 9:36:44 AM
> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues
> 
> Quoting Tom St Denis <tstdenis@elliptictech.com>:
> 
> > Hey all,
> >
> > Here's an updated patch which addresses a couple of build issues
> > and
> > coding style complaints.
> >
> > I still can't get it to run via testmgr I get
> >
> > [  162.407807] alg: No test for cmac(aes) (cmac(aes-generic))
> >
> > Despite the fact I have an entry for cmac(aes) (much like
> > xcbc(aes)...).
> >
> > Here's the patch to bring 3.8-rc4 up with CMAC ...
> >
> > Signed-off-by: Tom St Denis <tstdenis@elliptictech.com>
> >
> <snip>
> > diff --git a/include/uapi/linux/pfkeyv2.h
> > b/include/uapi/linux/pfkeyv2.h
> > index 0b80c80..d61898e 100644
> > --- a/include/uapi/linux/pfkeyv2.h
> > +++ b/include/uapi/linux/pfkeyv2.h
> > @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
> >  #define SADB_X_AALG_SHA2_512HMAC	7
> >  #define SADB_X_AALG_RIPEMD160HMAC	8
> >  #define SADB_X_AALG_AES_XCBC_MAC	9
> > +#define SADB_X_AALG_AES_CMAC_MAC	10
> >  #define SADB_X_AALG_NULL		251	/* kame */
> >  #define SADB_AALG_MAX			251
> 
> Should these values be based on IANA assigned IPSEC AH transform
> identifiers?
> 
> https://www.iana.org/assignments/isakmp-registry/isakmp-registry.xml#isakmp-registry-6

There is no CMAC entry apparently ... despite the fact that CMAC is a proposed RFC standard for IPsec.

It might be safer to move that to 14 since it's currently unassigned and then go through whatever channels are required to allocate it.  Mostly this affects key setting.  So this means my patch would break AH_RSA setkey calls (which the kernel doesn't support anyways).

Tom

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Jussi Kivilinna]

Quoting Tom St Denis <tstdenis@elliptictech.com>:

> ----- Original Message -----
>> From: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
>> To: "Tom St Denis" <tstdenis@elliptictech.com>
>> Cc: linux-kernel@vger.kernel.org, "Herbert Xu"  
>> <herbert@gondor.apana.org.au>, "David Miller" <davem@davemloft.net>,
>> linux-crypto@vger.kernel.org, "Steffen Klassert"  
>> <steffen.klassert@secunet.com>, netdev@vger.kernel.org
>> Sent: Wednesday, 23 January, 2013 9:36:44 AM
>> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch  
>> issues, indent, and testmgr build issues
>>
>> Quoting Tom St Denis <tstdenis@elliptictech.com>:
>>
>> > Hey all,
>> >
>> > Here's an updated patch which addresses a couple of build issues
>> > and
>> > coding style complaints.
>> >
>> > I still can't get it to run via testmgr I get
>> >
>> > [  162.407807] alg: No test for cmac(aes) (cmac(aes-generic))
>> >
>> > Despite the fact I have an entry for cmac(aes) (much like
>> > xcbc(aes)...).
>> >
>> > Here's the patch to bring 3.8-rc4 up with CMAC ...
>> >
>> > Signed-off-by: Tom St Denis <tstdenis@elliptictech.com>
>> >
>> <snip>
>> > diff --git a/include/uapi/linux/pfkeyv2.h
>> > b/include/uapi/linux/pfkeyv2.h
>> > index 0b80c80..d61898e 100644
>> > --- a/include/uapi/linux/pfkeyv2.h
>> > +++ b/include/uapi/linux/pfkeyv2.h
>> > @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
>> >  #define SADB_X_AALG_SHA2_512HMAC	7
>> >  #define SADB_X_AALG_RIPEMD160HMAC	8
>> >  #define SADB_X_AALG_AES_XCBC_MAC	9
>> > +#define SADB_X_AALG_AES_CMAC_MAC	10
>> >  #define SADB_X_AALG_NULL		251	/* kame */
>> >  #define SADB_AALG_MAX			251
>>
>> Should these values be based on IANA assigned IPSEC AH transform
>> identifiers?
>>
>> https://www.iana.org/assignments/isakmp-registry/isakmp-registry.xml#isakmp-registry-6
>
> There is no CMAC entry apparently ... despite the fact that CMAC is  
> a proposed RFC standard for IPsec.
>
> It might be safer to move that to 14 since it's currently unassigned  
> and then go through whatever channels are required to allocate it.   
> Mostly this affects key setting.  So this means my patch would break  
> AH_RSA setkey calls (which the kernel doesn't support anyways).
>

Problem seems to be that PFKEYv2 does not quite work with IKEv2, and  
XFRM API should be used instead. There is new numbers assigned for  
IKEv2:  
https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7

For new SADB_X_AALG_*, I'd think you should use value from "Reserved  
for private use" range. Maybe 250?

But maybe better solution might be to not make AES-CMAC (or other new  
algorithms) available throught PFKEY API at all, just XFRM?

-Jussi

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Tom St Denis]

----- Original Message -----
> From: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: linux-kernel@vger.kernel.org, "Herbert Xu" <herbert@gondor.apana.org.au>, "David Miller" <davem@davemloft.net>,
> linux-crypto@vger.kernel.org, "Steffen Klassert" <steffen.klassert@secunet.com>, netdev@vger.kernel.org
> Sent: Wednesday, 23 January, 2013 10:35:10 AM
> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues
> 
> Quoting Tom St Denis <tstdenis@elliptictech.com>:
> 
> > ----- Original Message -----
> >> From: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
> >> To: "Tom St Denis" <tstdenis@elliptictech.com>
> >> Cc: linux-kernel@vger.kernel.org, "Herbert Xu"
> >> <herbert@gondor.apana.org.au>, "David Miller"
> >> <davem@davemloft.net>,
> >> linux-crypto@vger.kernel.org, "Steffen Klassert"
> >> <steffen.klassert@secunet.com>, netdev@vger.kernel.org
> >> Sent: Wednesday, 23 January, 2013 9:36:44 AM
> >> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch
> >> issues, indent, and testmgr build issues
> >>
> >> Quoting Tom St Denis <tstdenis@elliptictech.com>:
> >>
> >> > Hey all,
> >> >
> >> > Here's an updated patch which addresses a couple of build issues
> >> > and
> >> > coding style complaints.
> >> >
> >> > I still can't get it to run via testmgr I get
> >> >
> >> > [  162.407807] alg: No test for cmac(aes) (cmac(aes-generic))
> >> >
> >> > Despite the fact I have an entry for cmac(aes) (much like
> >> > xcbc(aes)...).
> >> >
> >> > Here's the patch to bring 3.8-rc4 up with CMAC ...
> >> >
> >> > Signed-off-by: Tom St Denis <tstdenis@elliptictech.com>
> >> >
> >> <snip>
> >> > diff --git a/include/uapi/linux/pfkeyv2.h
> >> > b/include/uapi/linux/pfkeyv2.h
> >> > index 0b80c80..d61898e 100644
> >> > --- a/include/uapi/linux/pfkeyv2.h
> >> > +++ b/include/uapi/linux/pfkeyv2.h
> >> > @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
> >> >  #define SADB_X_AALG_SHA2_512HMAC	7
> >> >  #define SADB_X_AALG_RIPEMD160HMAC	8
> >> >  #define SADB_X_AALG_AES_XCBC_MAC	9
> >> > +#define SADB_X_AALG_AES_CMAC_MAC	10
> >> >  #define SADB_X_AALG_NULL		251	/* kame */
> >> >  #define SADB_AALG_MAX			251
> >>
> >> Should these values be based on IANA assigned IPSEC AH transform
> >> identifiers?
> >>
> >> https://www.iana.org/assignments/isakmp-registry/isakmp-registry.xml#isakmp-registry-6
> >
> > There is no CMAC entry apparently ... despite the fact that CMAC is
> > a proposed RFC standard for IPsec.
> >
> > It might be safer to move that to 14 since it's currently
> > unassigned
> > and then go through whatever channels are required to allocate it.
> > Mostly this affects key setting.  So this means my patch would
> > break
> > AH_RSA setkey calls (which the kernel doesn't support anyways).
> >
> 
> Problem seems to be that PFKEYv2 does not quite work with IKEv2, and
> XFRM API should be used instead. There is new numbers assigned for
> IKEv2:
> https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
> 
> For new SADB_X_AALG_*, I'd think you should use value from "Reserved
> for private use" range. Maybe 250?
> 
> But maybe better solution might be to not make AES-CMAC (or other new
> algorithms) available throught PFKEY API at all, just XFRM?

This is where I'm getting out of my area of knowledge.  Internally we use the "ip" tool to set SAs and that's about all I know about key management for IPsec in the kernel.

For testing we only use static keys since IKE is not part of our hardware offering anyways.

As long as I can use "ip" to set the keys I'm ok with whatever changes we make to this entry.

Tom

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[YOSHIFUJI Hideaki]

Jussi Kivilinna wrote:

>>> > diff --git a/include/uapi/linux/pfkeyv2.h
>>> > b/include/uapi/linux/pfkeyv2.h
>>> > index 0b80c80..d61898e 100644
>>> > --- a/include/uapi/linux/pfkeyv2.h
>>> > +++ b/include/uapi/linux/pfkeyv2.h
>>> > @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
>>> >  #define SADB_X_AALG_SHA2_512HMAC    7
>>> >  #define SADB_X_AALG_RIPEMD160HMAC    8
>>> >  #define SADB_X_AALG_AES_XCBC_MAC    9
>>> > +#define SADB_X_AALG_AES_CMAC_MAC    10
>>> >  #define SADB_X_AALG_NULL        251    /* kame */
>>> >  #define SADB_AALG_MAX            251
>>>
>>> Should these values be based on IANA assigned IPSEC AH transform
>>> identifiers?
>>>
>>> https://www.iana.org/assignments/isakmp-registry/isakmp-registry.xml#isakmp-registry-6
>>
>> There is no CMAC entry apparently ... despite the fact that CMAC is a proposed RFC standard for IPsec.
>>
>> It might be safer to move that to 14 since it's currently unassigned and then go through whatever channels are required to allocate it.  Mostly this affects key setting.  So this means my patch would break AH_RSA setkey calls (which the kernel doesn't support anyways).
>>
> 
> Problem seems to be that PFKEYv2 does not quite work with IKEv2, and XFRM API should be used instead. There is new numbers assigned for IKEv2: https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
> 
> For new SADB_X_AALG_*, I'd think you should use value from "Reserved for private use" range. Maybe 250?

We can choose any value unless we do not break existing
binaries.  When IKE used, the daemon is responsible
for translation.

--yoshfuji

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[YOSHIFUJI Hideaki]

YOSHIFUJI Hideaki wrote:
> Jussi Kivilinna wrote:
> 
>>>>> diff --git a/include/uapi/linux/pfkeyv2.h
>>>>> b/include/uapi/linux/pfkeyv2.h
>>>>> index 0b80c80..d61898e 100644
>>>>> --- a/include/uapi/linux/pfkeyv2.h
>>>>> +++ b/include/uapi/linux/pfkeyv2.h
>>>>> @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
>>>>>  #define SADB_X_AALG_SHA2_512HMAC    7
>>>>>  #define SADB_X_AALG_RIPEMD160HMAC    8
>>>>>  #define SADB_X_AALG_AES_XCBC_MAC    9
>>>>> +#define SADB_X_AALG_AES_CMAC_MAC    10
>>>>>  #define SADB_X_AALG_NULL        251    /* kame */
>>>>>  #define SADB_AALG_MAX            251
>>>>
>>>> Should these values be based on IANA assigned IPSEC AH transform
>>>> identifiers?
>>>>
>>>> https://www.iana.org/assignments/isakmp-registry/isakmp-registry.xml#isakmp-registry-6
>>>
>>> There is no CMAC entry apparently ... despite the fact that CMAC is a proposed RFC standard for IPsec.
>>>
>>> It might be safer to move that to 14 since it's currently unassigned and then go through whatever channels are required to allocate it.  Mostly this affects key setting.  So this means my patch would break AH_RSA setkey calls (which the kernel doesn't support anyways).
>>>
>>
>> Problem seems to be that PFKEYv2 does not quite work with IKEv2, and XFRM API should be used instead. There is new numbers assigned for IKEv2: https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
>>
>> For new SADB_X_AALG_*, I'd think you should use value from "Reserved for private use" range. Maybe 250?
> 
> We can choose any value unless we do not break existing
> binaries.  When IKE used, the daemon is responsible
> for translation.

I meant, we can choose any values "if" we do not break ...

--yoshfuji

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Jussi Kivilinna]

Quoting YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>:

> YOSHIFUJI Hideaki wrote:
>> Jussi Kivilinna wrote:
>>
>>>>>> diff --git a/include/uapi/linux/pfkeyv2.h
>>>>>> b/include/uapi/linux/pfkeyv2.h
>>>>>> index 0b80c80..d61898e 100644
>>>>>> --- a/include/uapi/linux/pfkeyv2.h
>>>>>> +++ b/include/uapi/linux/pfkeyv2.h
>>>>>> @@ -296,6 +296,7 @@ struct sadb_x_kmaddress {
>>>>>>  #define SADB_X_AALG_SHA2_512HMAC    7
>>>>>>  #define SADB_X_AALG_RIPEMD160HMAC    8
>>>>>>  #define SADB_X_AALG_AES_XCBC_MAC    9
>>>>>> +#define SADB_X_AALG_AES_CMAC_MAC    10
>>>>>>  #define SADB_X_AALG_NULL        251    /* kame */
>>>>>>  #define SADB_AALG_MAX            251
>>>>>
>>>>> Should these values be based on IANA assigned IPSEC AH transform
>>>>> identifiers?
>>>>>
>>>>> https://www.iana.org/assignments/isakmp-registry/isakmp-registry.xml#isakmp-registry-6
>>>>
>>>> There is no CMAC entry apparently ... despite the fact that CMAC  
>>>> is a proposed RFC standard for IPsec.
>>>>
>>>> It might be safer to move that to 14 since it's currently  
>>>> unassigned and then go through whatever channels are required to  
>>>> allocate it.  Mostly this affects key setting.  So this means my  
>>>> patch would break AH_RSA setkey calls (which the kernel doesn't  
>>>> support anyways).
>>>>
>>>
>>> Problem seems to be that PFKEYv2 does not quite work with IKEv2,  
>>> and XFRM API should be used instead. There is new numbers assigned  
>>> for IKEv2:  
>>> https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
>>>
>>> For new SADB_X_AALG_*, I'd think you should use value from  
>>> "Reserved for private use" range. Maybe 250?
>>
>> We can choose any value unless we do not break existing
>> binaries.  When IKE used, the daemon is responsible
>> for translation.
>
> I meant, we can choose any values "if" we do not break ...
>

Ok, so giving '10' to AES-CMAC is fine after all?

And if I'd want to add Camellia-CTR and Camellia-CCM support, I can  
choose next free numbers from SADB_X_EALG_*?

-Jussi

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

On Wed, Jan 23, 2013 at 05:35:10PM +0200, Jussi Kivilinna wrote:
> 
> Problem seems to be that PFKEYv2 does not quite work with IKEv2, and
> XFRM API should be used instead. There is new numbers assigned for
> IKEv2: https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
> 
> For new SADB_X_AALG_*, I'd think you should use value from "Reserved
> for private use" range. Maybe 250?

This would be an option, but we have just a few slots for private
algorithms.

> 
> But maybe better solution might be to not make AES-CMAC (or other
> new algorithms) available throught PFKEY API at all, just XFRM?
> 

It is probably the best to make new algorithms unavailable for pfkey
as long as they have no official ikev1 iana transform identifier.

But how to do that? Perhaps we can assign SADB_X_AALG_NOPFKEY to
the private value 255 and return -EINVAL if pfkey tries to register
such an algorithm. The netlink interface does not use these
identifiers, everything should work as expected. So it should be
possible to use these algoritms with iproute2 and the most modern
ike deamons.

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Jussi Kivilinna]

Quoting Steffen Klassert <steffen.klassert@secunet.com>:

> On Wed, Jan 23, 2013 at 05:35:10PM +0200, Jussi Kivilinna wrote:
>>
>> Problem seems to be that PFKEYv2 does not quite work with IKEv2, and
>> XFRM API should be used instead. There is new numbers assigned for
>> IKEv2: 
>> https://www.iana.org/assignments/ikev2-parameters/ikev2-parameters.xml#ikev2-parameters-7
>>
>> For new SADB_X_AALG_*, I'd think you should use value from "Reserved
>> for private use" range. Maybe 250?
>
> This would be an option, but we have just a few slots for private
> algorithms.
>
>>
>> But maybe better solution might be to not make AES-CMAC (or other
>> new algorithms) available throught PFKEY API at all, just XFRM?
>>
>
> It is probably the best to make new algorithms unavailable for pfkey
> as long as they have no official ikev1 iana transform identifier.
>
> But how to do that? Perhaps we can assign SADB_X_AALG_NOPFKEY to
> the private value 255 and return -EINVAL if pfkey tries to register
> such an algorithm. The netlink interface does not use these
> identifiers, everything should work as expected. So it should be
> possible to use these algoritms with iproute2 and the most modern
> ike deamons.

Maybe it would be cleaner to not mess with pfkeyv2.h at all, but instead mark algorithms that do not support pfkey with flag. See patch below.

Then I started looking up if sadb_alg_id is being used somewhere outside pfkey. Seems that its value is just being copied around.. but at "http://lxr.linux.no/linux+v3.7/net/xfrm/xfrm_policy.c#L1991" it's used as bit-index. So do larger values than 31 break some stuff? Can multiple algorithms have same sadb_alg_id value? Also in af_key.c, sadb_alg_id being used as bit-index.

-Jussi

---
ONLY COMPILE TESTED!
---
 include/net/xfrm.h   |    5 +++--
 net/key/af_key.c     |   39 +++++++++++++++++++++++++++++++--------
 net/xfrm/xfrm_algo.c |   12 ++++++------
 3 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 421f764..5d5eec2 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1320,6 +1320,7 @@ struct xfrm_algo_desc {
 	char *name;
 	char *compat;
 	u8 available:1;
+	u8 sadb_disabled:1;
 	union {
 		struct xfrm_algo_aead_info aead;
 		struct xfrm_algo_auth_info auth;
@@ -1561,8 +1562,8 @@ extern void xfrm_input_init(void);
 extern int xfrm_parse_spi(struct sk_buff *skb, u8 nexthdr, __be32 *spi, __be32 *seq);
 
 extern void xfrm_probe_algs(void);
-extern int xfrm_count_auth_supported(void);
-extern int xfrm_count_enc_supported(void);
+extern int xfrm_count_sadb_auth_supported(void);
+extern int xfrm_count_sadb_enc_supported(void);
 extern struct xfrm_algo_desc *xfrm_aalg_get_byidx(unsigned int idx);
 extern struct xfrm_algo_desc *xfrm_ealg_get_byidx(unsigned int idx);
 extern struct xfrm_algo_desc *xfrm_aalg_get_byid(int alg_id);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 5b426a6..307cf1d 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -816,18 +816,21 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 	sa->sadb_sa_auth = 0;
 	if (x->aalg) {
 		struct xfrm_algo_desc *a = xfrm_aalg_get_byname(x->aalg->alg_name, 0);
-		sa->sadb_sa_auth = a ? a->desc.sadb_alg_id : 0;
+		sa->sadb_sa_auth = (a && !a->sadb_disabled) ?
+					a->desc.sadb_alg_id : 0;
 	}
 	sa->sadb_sa_encrypt = 0;
 	BUG_ON(x->ealg && x->calg);
 	if (x->ealg) {
 		struct xfrm_algo_desc *a = xfrm_ealg_get_byname(x->ealg->alg_name, 0);
-		sa->sadb_sa_encrypt = a ? a->desc.sadb_alg_id : 0;
+		sa->sadb_sa_encrypt = (a && !a->sadb_disabled) ?
+					a->desc.sadb_alg_id : 0;
 	}
 	/* KAME compatible: sadb_sa_encrypt is overloaded with calg id */
 	if (x->calg) {
 		struct xfrm_algo_desc *a = xfrm_calg_get_byname(x->calg->alg_name, 0);
-		sa->sadb_sa_encrypt = a ? a->desc.sadb_alg_id : 0;
+		sa->sadb_sa_encrypt = (a && !a->sadb_disabled) ?
+					a->desc.sadb_alg_id : 0;
 	}
 
 	sa->sadb_sa_flags = 0;
@@ -1138,7 +1141,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 	if (sa->sadb_sa_auth) {
 		int keysize = 0;
 		struct xfrm_algo_desc *a = xfrm_aalg_get_byid(sa->sadb_sa_auth);
-		if (!a) {
+		if (!a || a->sadb_disabled) {
 			err = -ENOSYS;
 			goto out;
 		}
@@ -1160,7 +1163,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 	if (sa->sadb_sa_encrypt) {
 		if (hdr->sadb_msg_satype == SADB_X_SATYPE_IPCOMP) {
 			struct xfrm_algo_desc *a = xfrm_calg_get_byid(sa->sadb_sa_encrypt);
-			if (!a) {
+			if (!a || a->sadb_disabled) {
 				err = -ENOSYS;
 				goto out;
 			}
@@ -1172,7 +1175,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 		} else {
 			int keysize = 0;
 			struct xfrm_algo_desc *a = xfrm_ealg_get_byid(sa->sadb_sa_encrypt);
-			if (!a) {
+			if (!a || a->sadb_disabled) {
 				err = -ENOSYS;
 				goto out;
 			}
@@ -1578,13 +1581,13 @@ static struct sk_buff *compose_sadb_supported(const struct sadb_msg *orig,
 	struct sadb_msg *hdr;
 	int len, auth_len, enc_len, i;
 
-	auth_len = xfrm_count_auth_supported();
+	auth_len = xfrm_count_sadb_auth_supported();
 	if (auth_len) {
 		auth_len *= sizeof(struct sadb_alg);
 		auth_len += sizeof(struct sadb_supported);
 	}
 
-	enc_len = xfrm_count_enc_supported();
+	enc_len = xfrm_count_sadb_enc_supported();
 	if (enc_len) {
 		enc_len *= sizeof(struct sadb_alg);
 		enc_len += sizeof(struct sadb_supported);
@@ -1615,6 +1618,8 @@ static struct sk_buff *compose_sadb_supported(const struct sadb_msg *orig,
 			struct xfrm_algo_desc *aalg = xfrm_aalg_get_byidx(i);
 			if (!aalg)
 				break;
+			if (aalg->sadb_disabled)
+				continue;
 			if (aalg->available)
 				*ap++ = aalg->desc;
 		}
@@ -1634,6 +1639,8 @@ static struct sk_buff *compose_sadb_supported(const struct sadb_msg *orig,
 			struct xfrm_algo_desc *ealg = xfrm_ealg_get_byidx(i);
 			if (!ealg)
 				break;
+			if (ealg->sadb_disabled)
+				continue;
 			if (ealg->available)
 				*ap++ = ealg->desc;
 		}
@@ -2825,6 +2832,8 @@ static int count_ah_combs(const struct xfrm_tmpl *t)
 		const struct xfrm_algo_desc *aalg = xfrm_aalg_get_byidx(i);
 		if (!aalg)
 			break;
+		if (aalg->sadb_disabled)
+			continue;
 		if (aalg_tmpl_set(t, aalg) && aalg->available)
 			sz += sizeof(struct sadb_comb);
 	}
@@ -2840,6 +2849,9 @@ static int count_esp_combs(const struct xfrm_tmpl *t)
 		if (!ealg)
 			break;
 
+		if (ealg->sadb_disabled)
+			continue;
+
 		if (!(ealg_tmpl_set(t, ealg) && ealg->available))
 			continue;
 
@@ -2848,6 +2860,9 @@ static int count_esp_combs(const struct xfrm_tmpl *t)
 			if (!aalg)
 				break;
 
+			if (aalg->sadb_disabled)
+				continue;
+
 			if (aalg_tmpl_set(t, aalg) && aalg->available)
 				sz += sizeof(struct sadb_comb);
 		}
@@ -2871,6 +2886,9 @@ static void dump_ah_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 		if (!aalg)
 			break;
 
+		if (aalg->sadb_disabled)
+			continue;
+
 		if (aalg_tmpl_set(t, aalg) && aalg->available) {
 			struct sadb_comb *c;
 			c = (struct sadb_comb*)skb_put(skb, sizeof(struct sadb_comb));
@@ -2903,6 +2921,9 @@ static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 		if (!ealg)
 			break;
 
+		if (ealg->sadb_disabled)
+			continue;
+
 		if (!(ealg_tmpl_set(t, ealg) && ealg->available))
 			continue;
 
@@ -2911,6 +2932,8 @@ static void dump_esp_combs(struct sk_buff *skb, const struct xfrm_tmpl *t)
 			const struct xfrm_algo_desc *aalg = xfrm_aalg_get_byidx(k);
 			if (!aalg)
 				break;
+			if (aalg->sadb_disabled)
+				continue;
 			if (!(aalg_tmpl_set(t, aalg) && aalg->available))
 				continue;
 			c = (struct sadb_comb*)skb_put(skb, sizeof(struct sadb_comb));
diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
index 4694cca..dab881c 100644
--- a/net/xfrm/xfrm_algo.c
+++ b/net/xfrm/xfrm_algo.c
@@ -713,27 +713,27 @@ void xfrm_probe_algs(void)
 }
 EXPORT_SYMBOL_GPL(xfrm_probe_algs);
 
-int xfrm_count_auth_supported(void)
+int xfrm_count_sadb_auth_supported(void)
 {
 	int i, n;
 
 	for (i = 0, n = 0; i < aalg_entries(); i++)
-		if (aalg_list[i].available)
+		if (aalg_list[i].available && !aalg_list[i].sadb_disabled)
 			n++;
 	return n;
 }
-EXPORT_SYMBOL_GPL(xfrm_count_auth_supported);
+EXPORT_SYMBOL_GPL(xfrm_count_sadb_auth_supported);
 
-int xfrm_count_enc_supported(void)
+int xfrm_count_sadb_enc_supported(void)
 {
 	int i, n;
 
 	for (i = 0, n = 0; i < ealg_entries(); i++)
-		if (ealg_list[i].available)
+		if (ealg_list[i].available && !ealg_list[i].sadb_disabled)
 			n++;
 	return n;
 }
-EXPORT_SYMBOL_GPL(xfrm_count_enc_supported);
+EXPORT_SYMBOL_GPL(xfrm_count_sadb_enc_supported);
 
 #if defined(CONFIG_INET_ESP) || defined(CONFIG_INET_ESP_MODULE) || defined(CONFIG_INET6_ESP) || defined(CONFIG_INET6_ESP_MODULE)
 

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
> 
> Maybe it would be cleaner to not mess with pfkeyv2.h at all, but instead mark algorithms that do not support pfkey with flag. See patch below.
> 

Yes, would be an option too. I would be fine with that,
but let's here if someone else has an opinion on this.
Anyway, we need a solution to integrate Tom's patch soon.

> Then I started looking up if sadb_alg_id is being used somewhere outside pfkey. Seems that its value is just being copied around.. but at "http://lxr.linux.no/linux+v3.7/net/xfrm/xfrm_policy.c#L1991" it's used as bit-index. So do larger values than 31 break some stuff? Can multiple algorithms have same sadb_alg_id value? Also in af_key.c, sadb_alg_id being used as bit-index.
> 

Herbert tried to address this already in git commit c5d18e984
([IPSEC]: Fix catch-22 with algorithm IDs above 31) some years
ago.

But this looks still messy. If the aalgos, ealgos and calgos mask is ~0, 
we allow all algorithms. If this is not the case, xfrm and pfkey check
the aalgos mask against the algorithm ID, only pfkey checks the ealgo
mask and noone checks the calgos mask.

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Tom St Denis]

----- Original Message -----
> From: "Steffen Klassert" <steffen.klassert@secunet.com>
> To: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
> Cc: "Herbert Xu" <herbert@gondor.apana.org.au>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-crypto@vger.kernel.org, "Tom St Denis" <tstdenis@elliptictech.com>, "David Miller" <davem@davemloft.net>
> Sent: Thursday, 24 January, 2013 7:32:10 AM
> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues
> 
> On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
> > 
> > Maybe it would be cleaner to not mess with pfkeyv2.h at all, but
> > instead mark algorithms that do not support pfkey with flag. See
> > patch below.
> > 
> 
> Yes, would be an option too. I would be fine with that,
> but let's here if someone else has an opinion on this.
> Anyway, we need a solution to integrate Tom's patch soon.

Has anyone addressed the testmgr issues too?  I wasn't able to run the self-tests so I don't even know if the vectors were copied correctly.

Also a question for the netdev folk, in order to be timely would it be acceptable to patch ah4 and then ah6 with the AEAD changes?  Or would the team require both to be patched simultaneously?
 
Tom

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

On Thu, Jan 24, 2013 at 07:37:50AM -0500, Tom St Denis wrote:
> 
> Also a question for the netdev folk, in order to be timely would it be acceptable to patch ah4 and then ah6 with the AEAD changes?  Or would the team require both to be patched simultaneously?
>  

We would need patches for both, but the changes to ah4/ah6 should
e very similar.

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Tom St Denis]

----- Original Message -----
> From: "Steffen Klassert" <steffen.klassert@secunet.com>
> To: "Tom St Denis" <tstdenis@elliptictech.com>
> Cc: "Herbert Xu" <herbert@gondor.apana.org.au>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
> linux-crypto@vger.kernel.org, "David Miller" <davem@davemloft.net>, "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
> Sent: Thursday, 24 January, 2013 7:52:34 AM
> Subject: Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues
> 
> On Thu, Jan 24, 2013 at 07:37:50AM -0500, Tom St Denis wrote:
> > 
> > Also a question for the netdev folk, in order to be timely would it
> > be acceptable to patch ah4 and then ah6 with the AEAD changes?  Or
> > would the team require both to be patched simultaneously?
> >  
> 
> We would need patches for both, but the changes to ah4/ah6 should
> e very similar.

I suspect, I just don't know how my time table will look like so I was trying to not block the submission of one by the other.  Fortunately, I don't need hardware to test these changes so I can probably skunkwork them from home.

Tom

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Steffen Klassert]

On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
> 
> Maybe it would be cleaner to not mess with pfkeyv2.h at all, but instead mark algorithms that do not support pfkey with flag. See patch below.
> 

As nobody seems to have another opinion, we could go either with your
approach, or we can invert the logic and mark all existing algorithms
as pfkey supported. Then we would not need to bother about pfkey again.

I'd be fine with both. Do you want to submit a patch?

Re: [PATCH] CMAC support for CryptoAPI, fixed patch issues, indent, and testmgr build issues

[Jussi Kivilinna]

Quoting Steffen Klassert <steffen.klassert@secunet.com>:

> On Thu, Jan 24, 2013 at 01:25:46PM +0200, Jussi Kivilinna wrote:
>>
>> Maybe it would be cleaner to not mess with pfkeyv2.h at all, but  
>> instead mark algorithms that do not support pfkey with flag. See  
>> patch below.
>>
>
> As nobody seems to have another opinion, we could go either with your
> approach, or we can invert the logic and mark all existing algorithms
> as pfkey supported. Then we would not need to bother about pfkey again.
>
> I'd be fine with both. Do you want to submit a patch?
>

Ok, I'll invert the logic and send new patch.

-Jussi