| Message ID | 20190611134750.2974-6-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: rc4 cleanup | expand |
On Tue, Jun 11, 2019 at 03:47:48PM +0200, Ard Biesheuvel wrote: > There are no remaining users of the cipher implementation, and there > are no meaningful ways in which the arc4 cipher can be combined with > templates other than ECB (and the way we do provide that combination > is highly dubious to begin with). > > So let's drop the arc4 cipher altogether, and only keep the ecb(arc4) > skcipher, which is used in various places in the kernel. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > crypto/arc4.c | 46 ++------------------ > 1 file changed, 4 insertions(+), 42 deletions(-) > > diff --git a/crypto/arc4.c b/crypto/arc4.c > index 6974dba1b7b9..79a51e9f90ae 100644 > --- a/crypto/arc4.c > +++ b/crypto/arc4.c > @@ -13,23 +13,12 @@ > #include <linux/init.h> > #include <linux/module.h> > > -static int arc4_set_key(struct crypto_tfm *tfm, const u8 *in_key, > - unsigned int key_len) > -{ > - struct arc4_ctx *ctx = crypto_tfm_ctx(tfm); > - > - return arc4_setkey(ctx, in_key, key_len); > -} > - > static int arc4_set_key_skcipher(struct crypto_skcipher *tfm, const u8 *in_key, > unsigned int key_len) > { > - return arc4_set_key(&tfm->base, in_key, key_len); > -} > + struct arc4_ctx *ctx = crypto_tfm_ctx(&tfm->base); > > -static void arc4_crypt_one(struct crypto_tfm *tfm, u8 *out, const u8 *in) > -{ > - arc4_crypt(crypto_tfm_ctx(tfm), out, in, 1); > + return arc4_setkey(ctx, in_key, key_len); > } > > static int ecb_arc4_crypt(struct skcipher_request *req) Can you clean up the naming here? arc4_set_key_skcipher() => crypto_arc4_setkey() ecb_arc4_crypt() => crypto_arc4_crypt() The current names were intended to distinguish the "skcipher" functions from the "cipher" functions, but that will no longer be needed. Also, crypto_arc4_setkey() should use crypto_skcipher_ctx() rather than crypto_tfm_ctx(), now that it only handles "skcipher". > @@ -50,23 +39,6 @@ static int ecb_arc4_crypt(struct skcipher_request *req) > return err; > } > > -static struct crypto_alg arc4_cipher = { > - .cra_name = "arc4", > - .cra_flags = CRYPTO_ALG_TYPE_CIPHER, > - .cra_blocksize = ARC4_BLOCK_SIZE, > - .cra_ctxsize = sizeof(struct arc4_ctx), > - .cra_module = THIS_MODULE, > - .cra_u = { > - .cipher = { > - .cia_min_keysize = ARC4_MIN_KEY_SIZE, > - .cia_max_keysize = ARC4_MAX_KEY_SIZE, > - .cia_setkey = arc4_set_key, > - .cia_encrypt = arc4_crypt_one, > - .cia_decrypt = arc4_crypt_one, > - }, > - }, > -}; > - > static struct skcipher_alg arc4_skcipher = { Similarly this could be renamed from arc4_skcipher to arc4_alg, now that the skcipher algorithm doesn't need to be distinguished from the cipher algorithm. > .base.cra_name = "ecb(arc4)", Given the confusion this name causes, can you leave a comment? Like: /* * For legacy reasons, this is named "ecb(arc4)", not "arc4". * Nevertheless it's actually a stream cipher, not a block cipher. */ .base.cra_name = "ecb(arc4)", Also, due to removing the cipher algorithm, we need the following testmgr change so that the comparison self-tests consider the generic implementation of this algorithm to be itself rather than "ecb(arc4-generic)": diff --git a/crypto/testmgr.c b/crypto/testmgr.c index 658a7eeebab28..5d3eb8577605f 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -4125,6 +4125,7 @@ static const struct alg_test_desc alg_test_descs[] = { } }, { .alg = "ecb(arc4)", + .generic_driver = "ecb(arc4)-generic", .test = alg_test_skcipher, .suite = { .cipher = __VECS(arc4_tv_template) - Eric
On Tue, Jun 11, 2019 at 10:39:39AM -0700, Eric Biggers wrote: > > - > > static struct skcipher_alg arc4_skcipher = { > > Similarly this could be renamed from arc4_skcipher to arc4_alg, now that the > skcipher algorithm doesn't need to be distinguished from the cipher algorithm. > > > .base.cra_name = "ecb(arc4)", > > Given the confusion this name causes, can you leave a comment? Like: > > /* > * For legacy reasons, this is named "ecb(arc4)", not "arc4". > * Nevertheless it's actually a stream cipher, not a block cipher. > */ > .base.cra_name = "ecb(arc4)", > > > Also, due to removing the cipher algorithm, we need the following testmgr change > so that the comparison self-tests consider the generic implementation of this > algorithm to be itself rather than "ecb(arc4-generic)": > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c > index 658a7eeebab28..5d3eb8577605f 100644 > --- a/crypto/testmgr.c > +++ b/crypto/testmgr.c > @@ -4125,6 +4125,7 @@ static const struct alg_test_desc alg_test_descs[] = { > } > }, { > .alg = "ecb(arc4)", > + .generic_driver = "ecb(arc4)-generic", > .test = alg_test_skcipher, > .suite = { > .cipher = __VECS(arc4_tv_template) > > - Eric Hi Ard, did you see these comments? They weren't addressed in v4. We need at least the testmgr change, otherwise there's a warning when booting with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y: [ 0.542610] alg: skcipher: skipping comparison tests for ecb(arc4)-generic because ecb(arc4-generic) is unavailable - Eric
On Wed, 12 Jun 2019 at 17:33, Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jun 11, 2019 at 10:39:39AM -0700, Eric Biggers wrote: > > > - > > > static struct skcipher_alg arc4_skcipher = { > > > > Similarly this could be renamed from arc4_skcipher to arc4_alg, now that the > > skcipher algorithm doesn't need to be distinguished from the cipher algorithm. > > > > > .base.cra_name = "ecb(arc4)", > > > > Given the confusion this name causes, can you leave a comment? Like: > > > > /* > > * For legacy reasons, this is named "ecb(arc4)", not "arc4". > > * Nevertheless it's actually a stream cipher, not a block cipher. > > */ > > .base.cra_name = "ecb(arc4)", > > > > > > Also, due to removing the cipher algorithm, we need the following testmgr change > > so that the comparison self-tests consider the generic implementation of this > > algorithm to be itself rather than "ecb(arc4-generic)": > > > > diff --git a/crypto/testmgr.c b/crypto/testmgr.c > > index 658a7eeebab28..5d3eb8577605f 100644 > > --- a/crypto/testmgr.c > > +++ b/crypto/testmgr.c > > @@ -4125,6 +4125,7 @@ static const struct alg_test_desc alg_test_descs[] = { > > } > > }, { > > .alg = "ecb(arc4)", > > + .generic_driver = "ecb(arc4)-generic", > > .test = alg_test_skcipher, > > .suite = { > > .cipher = __VECS(arc4_tv_template) > > > > - Eric > > Hi Ard, did you see these comments? They weren't addressed in v4. We need at > least the testmgr change, otherwise there's a warning when booting with > CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y: > > [ 0.542610] alg: skcipher: skipping comparison tests for ecb(arc4)-generic because ecb(arc4-generic) is unavailable > Oops, no, I didn't I'll fix it up and resend. I forgot to add a MODULE_LICENSE() to libarc4.ko as well, so I needed to do this anyway.
diff --git a/crypto/arc4.c b/crypto/arc4.c index 6974dba1b7b9..79a51e9f90ae 100644 --- a/crypto/arc4.c +++ b/crypto/arc4.c @@ -13,23 +13,12 @@ #include <linux/init.h> #include <linux/module.h> -static int arc4_set_key(struct crypto_tfm *tfm, const u8 *in_key, - unsigned int key_len) -{ - struct arc4_ctx *ctx = crypto_tfm_ctx(tfm); - - return arc4_setkey(ctx, in_key, key_len); -} - static int arc4_set_key_skcipher(struct crypto_skcipher *tfm, const u8 *in_key, unsigned int key_len) { - return arc4_set_key(&tfm->base, in_key, key_len); -} + struct arc4_ctx *ctx = crypto_tfm_ctx(&tfm->base); -static void arc4_crypt_one(struct crypto_tfm *tfm, u8 *out, const u8 *in) -{ - arc4_crypt(crypto_tfm_ctx(tfm), out, in, 1); + return arc4_setkey(ctx, in_key, key_len); } static int ecb_arc4_crypt(struct skcipher_request *req) @@ -50,23 +39,6 @@ static int ecb_arc4_crypt(struct skcipher_request *req) return err; } -static struct crypto_alg arc4_cipher = { - .cra_name = "arc4", - .cra_flags = CRYPTO_ALG_TYPE_CIPHER, - .cra_blocksize = ARC4_BLOCK_SIZE, - .cra_ctxsize = sizeof(struct arc4_ctx), - .cra_module = THIS_MODULE, - .cra_u = { - .cipher = { - .cia_min_keysize = ARC4_MIN_KEY_SIZE, - .cia_max_keysize = ARC4_MAX_KEY_SIZE, - .cia_setkey = arc4_set_key, - .cia_encrypt = arc4_crypt_one, - .cia_decrypt = arc4_crypt_one, - }, - }, -}; - static struct skcipher_alg arc4_skcipher = { .base.cra_name = "ecb(arc4)", .base.cra_priority = 100, @@ -82,21 +54,11 @@ static struct skcipher_alg arc4_skcipher = { static int __init arc4_init(void) { - int err; - - err = crypto_register_alg(&arc4_cipher); - if (err) - return err; - - err = crypto_register_skcipher(&arc4_skcipher); - if (err) - crypto_unregister_alg(&arc4_cipher); - return err; + return crypto_register_skcipher(&arc4_skcipher); } static void __exit arc4_exit(void) { - crypto_unregister_alg(&arc4_cipher); crypto_unregister_skcipher(&arc4_skcipher); } @@ -106,4 +68,4 @@ module_exit(arc4_exit); MODULE_LICENSE("GPL"); MODULE_DESCRIPTION("ARC4 Cipher Algorithm"); MODULE_AUTHOR("Jon Oberheide <jon@oberheide.org>"); -MODULE_ALIAS_CRYPTO("arc4"); +MODULE_ALIAS_CRYPTO("ecb(arc4)");
There are no remaining users of the cipher implementation, and there are no meaningful ways in which the arc4 cipher can be combined with templates other than ECB (and the way we do provide that combination is highly dubious to begin with). So let's drop the arc4 cipher altogether, and only keep the ecb(arc4) skcipher, which is used in various places in the kernel. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- crypto/arc4.c | 46 ++------------------ 1 file changed, 4 insertions(+), 42 deletions(-) -- 2.20.1