Re: [PATCH v3 5/7] crypto: arc4 - remove cipher implementation

[Eric Biggers <ebiggers@kernel.org>]

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