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 <[email protected]>
---
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
+ select CRYPTO_HASH
+ select CRYPTO_MANAGER
+ help
+ NIST CMAC cipher based MAC
+
config CRYPTO_VMAC
tristate "VMAC support"
depends on EXPERIMENTAL
diff --git a/crypto/Makefile b/crypto/Makefile
index d59dec7..85a615d 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_CRYPTO_USER) += crypto_user.o
obj-$(CONFIG_CRYPTO_HMAC) += hmac.o
obj-$(CONFIG_CRYPTO_VMAC) += vmac.o
obj-$(CONFIG_CRYPTO_XCBC) += xcbc.o
+obj-$(CONFIG_CRYPTO_CMAC) += cmac.o
obj-$(CONFIG_CRYPTO_NULL) += crypto_null.o
obj-$(CONFIG_CRYPTO_MD4) += md4.o
obj-$(CONFIG_CRYPTO_MD5) += md5.o
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
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Author:
+ * Tom St Denis <[email protected]>
+ *
+ * Derived from the XCBC code of:
+ * Kazunori Miyazawa <[email protected]>
+ */
+
+#include <crypto/internal/hash.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+/*
+ * +------------------------
+ * | <parent tfm>
+ * +------------------------
+ * | cmac_tfm_ctx
+ * +------------------------
+ * | consts (block size * 2)
+ * +------------------------
+ */
+struct cmac_tfm_ctx {
+ struct crypto_cipher *child;
+ u8 ctx[];
+};
+
+/*
+ * +------------------------
+ * | <shash desc>
+ * +------------------------
+ * | cmac_desc_ctx
+ * +------------------------
+ * | odds (block size)
+ * +------------------------
+ * | prev (block size)
+ * +------------------------
+ */
+struct cmac_desc_ctx {
+ unsigned int len;
+ u8 ctx[];
+};
+
+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 */
+ }
+
+ err = crypto_cipher_setkey(ctx->child, inkey, keylen);
+ if (err)
+ return err;
+
+ /* encrypt the zero block */
+ memset(consts, 0, bs);
+ crypto_cipher_encrypt_one(ctx->child, consts, consts);
+
+ for (x = 0; x < 2; x++) {
+ /* if msb(L * u^(x+1)) = 0 then just shift,
+ otherwise shift and xor constant mask */
+ msb = consts[x*bs + 0] >> 7;
+
+ /* 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;
+
+ consts[x*bs + bs - 1] =
+ ((consts[x*bs + bs - 1] << 1) ^
+ (msb ? mask : 0)) & 255;
+
+ /* copy up as require */
+ if (x == 0)
+ memcpy(&consts[(x+1)*bs], &consts[x*bs], bs);
+ }
+ return 0;
+}
+
+static int crypto_cmac_digest_init(struct shash_desc *pdesc)
+{
+ unsigned long alignmask = crypto_shash_alignmask(pdesc->tfm);
+ struct cmac_desc_ctx *ctx = shash_desc_ctx(pdesc);
+ int bs = crypto_shash_blocksize(pdesc->tfm);
+ u8 *prev = PTR_ALIGN(&ctx->ctx[0], alignmask + 1) + bs;
+
+ ctx->len = 0;
+ memset(prev, 0, bs);
+
+ return 0;
+}
+
+static int crypto_cmac_digest_update(struct shash_desc *pdesc, const u8 *p,
+ unsigned int len)
+{
+ struct crypto_shash *parent = pdesc->tfm;
+ unsigned long alignmask = crypto_shash_alignmask(parent);
+ struct cmac_tfm_ctx *tctx = crypto_shash_ctx(parent);
+ struct cmac_desc_ctx *ctx = shash_desc_ctx(pdesc);
+ struct crypto_cipher *tfm = tctx->child;
+ int bs = crypto_shash_blocksize(parent);
+ u8 *odds = PTR_ALIGN(&ctx->ctx[0], alignmask + 1);
+ u8 *prev = odds + bs;
+
+ /* checking the data can fill the block */
+ if ((ctx->len + len) <= bs) {
+ memcpy(odds + ctx->len, p, len);
+ ctx->len += len;
+ return 0;
+ }
+
+ /* filling odds with new data and encrypting it */
+ memcpy(odds + ctx->len, p, bs - ctx->len);
+ len -= bs - ctx->len;
+ p += bs - ctx->len;
+
+ crypto_xor(prev, odds, bs);
+ crypto_cipher_encrypt_one(tfm, prev, prev);
+
+ /* clearing the length */
+ ctx->len = 0;
+
+ /* encrypting the rest of data */
+ while (len > bs) {
+ crypto_xor(prev, p, bs);
+ crypto_cipher_encrypt_one(tfm, prev, prev);
+ p += bs;
+ len -= bs;
+ }
+
+ /* keeping the surplus of blocksize */
+ if (len) {
+ memcpy(odds, p, len);
+ ctx->len = len;
+ }
+
+ return 0;
+}
+
+static int crypto_cmac_digest_final(struct shash_desc *pdesc, u8 *out)
+{
+ struct crypto_shash *parent = pdesc->tfm;
+ unsigned long alignmask = crypto_shash_alignmask(parent);
+ struct cmac_tfm_ctx *tctx = crypto_shash_ctx(parent);
+ struct cmac_desc_ctx *ctx = shash_desc_ctx(pdesc);
+ struct crypto_cipher *tfm = tctx->child;
+ int bs = crypto_shash_blocksize(parent);
+ u8 *consts = PTR_ALIGN(&tctx->ctx[0], alignmask + 1);
+ u8 *odds = PTR_ALIGN(&ctx->ctx[0], alignmask + 1);
+ u8 *prev = odds + bs;
+ unsigned int offset = 0;
+
+ if (ctx->len != bs) {
+ unsigned int rlen;
+ u8 *p = odds + ctx->len;
+
+ *p = 0x80;
+ p++;
+
+ rlen = bs - ctx->len - 1;
+ if (rlen)
+ memset(p, 0, rlen);
+
+ offset += bs;
+ }
+
+ crypto_xor(prev, odds, bs);
+ crypto_xor(prev, consts + offset, bs);
+
+ crypto_cipher_encrypt_one(tfm, out, prev);
+
+ return 0;
+}
+
+static int cmac_init_tfm(struct crypto_tfm *tfm)
+{
+ struct crypto_cipher *cipher;
+ struct crypto_instance *inst = (void *)tfm->__crt_alg;
+ struct crypto_spawn *spawn = crypto_instance_ctx(inst);
+ struct cmac_tfm_ctx *ctx = crypto_tfm_ctx(tfm);
+
+ cipher = crypto_spawn_cipher(spawn);
+ if (IS_ERR(cipher))
+ return PTR_ERR(cipher);
+
+ ctx->child = cipher;
+
+ return 0;
+};
+
+static void cmac_exit_tfm(struct crypto_tfm *tfm)
+{
+ struct cmac_tfm_ctx *ctx = crypto_tfm_ctx(tfm);
+ crypto_free_cipher(ctx->child);
+}
+
+static int cmac_create(struct crypto_template *tmpl, struct rtattr **tb)
+{
+ struct shash_instance *inst;
+ struct crypto_alg *alg;
+ unsigned long alignmask;
+ int err;
+
+ err = crypto_check_attr_type(tb, CRYPTO_ALG_TYPE_SHASH);
+ if (err)
+ return err;
+
+ alg = crypto_get_attr_alg(tb, CRYPTO_ALG_TYPE_CIPHER,
+ CRYPTO_ALG_TYPE_MASK);
+ if (IS_ERR(alg))
+ return PTR_ERR(alg);
+
+ switch (alg->cra_blocksize) {
+ case 16:
+ break;
+ default:
+ goto out_put_alg;
+ }
+
+ inst = shash_alloc_instance("cmac", alg);
+ err = PTR_ERR(inst);
+ if (IS_ERR(inst))
+ goto out_put_alg;
+
+ err = crypto_init_spawn(shash_instance_ctx(inst), alg,
+ shash_crypto_instance(inst),
+ CRYPTO_ALG_TYPE_MASK);
+ if (err)
+ goto out_free_inst;
+
+ alignmask = alg->cra_alignmask | 3;
+ inst->alg.base.cra_alignmask = alignmask;
+ inst->alg.base.cra_priority = alg->cra_priority;
+ inst->alg.base.cra_blocksize = alg->cra_blocksize;
+
+ inst->alg.digestsize = alg->cra_blocksize;
+ inst->alg.descsize = ALIGN(sizeof(struct cmac_desc_ctx),
+ crypto_tfm_ctx_alignment()) +
+ (alignmask &
+ ~(crypto_tfm_ctx_alignment() - 1)) +
+ alg->cra_blocksize * 2;
+
+ inst->alg.base.cra_ctxsize = ALIGN(sizeof(struct cmac_tfm_ctx),
+ alignmask + 1) +
+ alg->cra_blocksize * 2;
+ inst->alg.base.cra_init = cmac_init_tfm;
+ inst->alg.base.cra_exit = cmac_exit_tfm;
+
+ inst->alg.init = crypto_cmac_digest_init;
+ inst->alg.update = crypto_cmac_digest_update;
+ inst->alg.final = crypto_cmac_digest_final;
+ inst->alg.setkey = crypto_cmac_digest_setkey;
+
+ err = shash_register_instance(tmpl, inst);
+ if (err) {
+out_free_inst:
+ shash_free_instance(shash_crypto_instance(inst));
+ }
+
+out_put_alg:
+ crypto_mod_put(alg);
+ return err;
+}
+
+static struct crypto_template crypto_cmac_tmpl = {
+ .name = "cmac",
+ .create = cmac_create,
+ .free = shash_free_instance,
+ .module = THIS_MODULE,
+};
+
+static int __init crypto_cmac_module_init(void)
+{
+ return crypto_register_template(&crypto_cmac_tmpl);
+}
+
+static void __exit crypto_cmac_module_exit(void)
+{
+ crypto_unregister_template(&crypto_cmac_tmpl);
+}
+
+module_init(crypto_cmac_module_init);
+module_exit(crypto_cmac_module_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("CMAC keyed hash algorithm");
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index edf4a08..6de7994 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2859,6 +2859,15 @@ static const struct alg_test_desc alg_test_descs[] = {
}
}
}, {
+ .alg = "cmac(aes)",
+ .test = alg_test_hash,
+ .suite = {
+ .hash = {
+ .vecs = aes_cmac128_tv_template,
+ .count = CMAC_AES_TEST_VECTORS
+ }
+ }
+ }, {
.alg = "xcbc(aes)",
.test = alg_test_hash,
.suite = {
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",
+ .plaintext = zeroed_string,
+ .digest = "\xbb\x1d\x69\x29\xe9\x59\x37\x28\x7f"
+ "\xa3\x7d\x12\x9b\x75\x67\x46",
+ .psize = 0,
+ .ksize = 16,
+ },
+
+ {
+ .key = "\x2b\x7e\x15\x16\x28\xae\xd2\xa6\xab\xf7"
+ "\x15\x88\x09\xcf\x4f\x3c",
+ .plaintext = "\x6b\xc1\xbe\xe2\x2e\x40\x9f\x96\xe9\x3d"
+ "\x7e\x11\x73\x93\x17\x2a",
+ .digest = "\x07\x0a\x16\xb4\x6b\x4d\x41\x44\xf7\x9b"
+ "\xdd\x9d\xd0\x4a\x28\x7c",
+ .psize = 16,
+ .ksize = 16,
+ },
+
+ {
+ .key = "\x2b\x7e\x15\x16\x28\xae\xd2\xa6\xab\xf7"
+ "\x15\x88\x09\xcf\x4f\x3c",
+ .plaintext = "\x6b\xc1\xbe\xe2\x2e\x40\x9f\x96\xe9"
+ "\x3d\x7e\x11\x73\x93\x17"
+ "\x2a\xae\x2d\x8a\x57\x1e\x03\xac\x9c\x9e\xb7\x6f\xac\x45\xaf"
+ "\x8e\x51\x30\xc8\x1c\x46\xa3\x5c\xe4\x11",
+ .digest = "\xdf\xa6\x67\x47\xde\x9a\xe6\x30\x30\xca\x32"
+ "\x61\x14\x97\xc8\x27",
+ .psize = 40,
+ .ksize = 16,
+ },
+ {
+ .key = "\x2b\x7e\x15\x16\x28\xae\xd2\xa6\xab\xf7"
+ "\x15\x88\x09\xcf\x4f\x3c",
+ .plaintext = "\x6b\xc1\xbe\xe2\x2e\x40\x9f\x96\xe9"
+ "\x3d\x7e\x11\x73\x93\x17"
+ "\x2a\xae\x2d\x8a\x57\x1e\x03\xac\x9c\x9e\xb7\x6f\xac\x45\xaf"
+ "\x8e\x51\x30\xc8\x1c\x46\xa3\x5c\xe4\x11\xe5\xfb\xc1\x19\x1a"
+ "\x0a\x52\xef\xf6\x9f\x24\x45\xdf\x4f\x9b\x17\xad\x2b\x41\x7b"
+ "\xe6\x6c\x37\x10",
+ .digest = "\x51\xf0\xbe\xbf\x7e\x3b\x9d\x92\xfc\x49\x74"
+ "\x17\x79\x36\x3c\xfe",
+ .psize = 64,
+ .ksize = 16,
+ },
+};
+
#define XCBC_AES_TEST_VECTORS 6
static struct hash_testvec aes_xcbc128_tv_template[] = {
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 = {
--
1.8.1.rc2.6.g18499ba
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 <[email protected]>
>
> ---
> 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 = {
[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 <[email protected]>
>
> ---
> 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 = {
(Cc [email protected])
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 <[email protected]>
>
> ---
> 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!
Quoting Tom St Denis <[email protected]>:
> 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 <[email protected]>
>
<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
----- Original Message -----
> From: "Jussi Kivilinna" <[email protected]>
> To: "Tom St Denis" <[email protected]>
> Cc: [email protected], "Herbert Xu" <[email protected]>, "David Miller" <[email protected]>,
> [email protected], "Steffen Klassert" <[email protected]>, [email protected]
> 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 <[email protected]>:
>
> > 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 <[email protected]>
> >
> <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
Quoting Tom St Denis <[email protected]>:
> ----- Original Message -----
>> From: "Jussi Kivilinna" <[email protected]>
>> To: "Tom St Denis" <[email protected]>
>> Cc: [email protected], "Herbert Xu"
>> <[email protected]>, "David Miller" <[email protected]>,
>> [email protected], "Steffen Klassert"
>> <[email protected]>, [email protected]
>> 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 <[email protected]>:
>>
>> > 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 <[email protected]>
>> >
>> <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
----- Original Message -----
> From: "Jussi Kivilinna" <[email protected]>
> To: "Tom St Denis" <[email protected]>
> Cc: [email protected], "Herbert Xu" <[email protected]>, "David Miller" <[email protected]>,
> [email protected], "Steffen Klassert" <[email protected]>, [email protected]
> 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 <[email protected]>:
>
> > ----- Original Message -----
> >> From: "Jussi Kivilinna" <[email protected]>
> >> To: "Tom St Denis" <[email protected]>
> >> Cc: [email protected], "Herbert Xu"
> >> <[email protected]>, "David Miller"
> >> <[email protected]>,
> >> [email protected], "Steffen Klassert"
> >> <[email protected]>, [email protected]
> >> 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 <[email protected]>:
> >>
> >> > 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 <[email protected]>
> >> >
> >> <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
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
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
Quoting YOSHIFUJI Hideaki <[email protected]>:
> 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
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.
Quoting Steffen Klassert <[email protected]>:
> 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)
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.
----- Original Message -----
> From: "Steffen Klassert" <[email protected]>
> To: "Jussi Kivilinna" <[email protected]>
> Cc: "Herbert Xu" <[email protected]>, [email protected], [email protected],
> [email protected], "Tom St Denis" <[email protected]>, "David Miller" <[email protected]>
> 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
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.
----- Original Message -----
> From: "Steffen Klassert" <[email protected]>
> To: "Tom St Denis" <[email protected]>
> Cc: "Herbert Xu" <[email protected]>, [email protected], [email protected],
> [email protected], "David Miller" <[email protected]>, "Jussi Kivilinna" <[email protected]>
> 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
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?
Quoting Steffen Klassert <[email protected]>:
> 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