[PATCH] crypto: caam - Remove broken arc4 support

[PATCH] crypto: caam - Remove broken arc4 support

[Herbert Xu]

The arc4 algorithm requires storing state in the request context
in order to allow more than one encrypt/decrypt operation.  As this
driver does not seem to do that, it means that using it for more
than one operation is broken.

Fixes: eaed71a44ad9 ("crypto: caam - add ecb(*) support")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index b2f9882bc010f..797bff9b93182 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -810,12 +810,6 @@ static int ctr_skcipher_setkey(struct crypto_skcipher *skcipher,
 	return skcipher_setkey(skcipher, key, keylen, ctx1_iv_off);
 }
 
-static int arc4_skcipher_setkey(struct crypto_skcipher *skcipher,
-				const u8 *key, unsigned int keylen)
-{
-	return skcipher_setkey(skcipher, key, keylen, 0);
-}
-
 static int des_skcipher_setkey(struct crypto_skcipher *skcipher,
 			       const u8 *key, unsigned int keylen)
 {
@@ -1967,21 +1961,6 @@ static struct caam_skcipher_alg driver_algs[] = {
 		},
 		.caam.class1_alg_type = OP_ALG_ALGSEL_3DES | OP_ALG_AAI_ECB,
 	},
-	{
-		.skcipher = {
-			.base = {
-				.cra_name = "ecb(arc4)",
-				.cra_driver_name = "ecb-arc4-caam",
-				.cra_blocksize = ARC4_BLOCK_SIZE,
-			},
-			.setkey = arc4_skcipher_setkey,
-			.encrypt = skcipher_encrypt,
-			.decrypt = skcipher_decrypt,
-			.min_keysize = ARC4_MIN_KEY_SIZE,
-			.max_keysize = ARC4_MAX_KEY_SIZE,
-		},
-		.caam.class1_alg_type = OP_ALG_ALGSEL_ARC4 | OP_ALG_AAI_ECB,
-	},
 };
 
 static struct caam_aead_alg driver_aeads[] = {
@@ -3457,7 +3436,6 @@ int caam_algapi_init(struct device *ctrldev)
 	struct caam_drv_private *priv = dev_get_drvdata(ctrldev);
 	int i = 0, err = 0;
 	u32 aes_vid, aes_inst, des_inst, md_vid, md_inst, ccha_inst, ptha_inst;
-	u32 arc4_inst;
 	unsigned int md_limit = SHA512_DIGEST_SIZE;
 	bool registered = false, gcm_support;
 
@@ -3477,8 +3455,6 @@ int caam_algapi_init(struct device *ctrldev)
 			   CHA_ID_LS_DES_SHIFT;
 		aes_inst = cha_inst & CHA_ID_LS_AES_MASK;
 		md_inst = (cha_inst & CHA_ID_LS_MD_MASK) >> CHA_ID_LS_MD_SHIFT;
-		arc4_inst = (cha_inst & CHA_ID_LS_ARC4_MASK) >>
-			    CHA_ID_LS_ARC4_SHIFT;
 		ccha_inst = 0;
 		ptha_inst = 0;
 
@@ -3499,7 +3475,6 @@ int caam_algapi_init(struct device *ctrldev)
 		md_inst = mdha & CHA_VER_NUM_MASK;
 		ccha_inst = rd_reg32(&priv->ctrl->vreg.ccha) & CHA_VER_NUM_MASK;
 		ptha_inst = rd_reg32(&priv->ctrl->vreg.ptha) & CHA_VER_NUM_MASK;
-		arc4_inst = rd_reg32(&priv->ctrl->vreg.afha) & CHA_VER_NUM_MASK;
 
 		gcm_support = aesa & CHA_VER_MISC_AES_GCM;
 	}
@@ -3522,10 +3497,6 @@ int caam_algapi_init(struct device *ctrldev)
 		if (!aes_inst && (alg_sel == OP_ALG_ALGSEL_AES))
 				continue;
 
-		/* Skip ARC4 algorithms if not supported by device */
-		if (!arc4_inst && alg_sel == OP_ALG_ALGSEL_ARC4)
-			continue;
-
 		/*
 		 * Check support for AES modes not available
 		 * on LP devices.
diff --git a/drivers/crypto/caam/compat.h b/drivers/crypto/caam/compat.h
index 60e2a54c19f11..c3c22a8de4c00 100644
--- a/drivers/crypto/caam/compat.h
+++ b/drivers/crypto/caam/compat.h
@@ -43,7 +43,6 @@
 #include <crypto/akcipher.h>
 #include <crypto/scatterwalk.h>
 #include <crypto/skcipher.h>
-#include <crypto/arc4.h>
 #include <crypto/internal/skcipher.h>
 #include <crypto/internal/hash.h>
 #include <crypto/internal/rsa.h>
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel]

On Thu, 2 Jul 2020 at 06:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> The arc4 algorithm requires storing state in the request context
> in order to allow more than one encrypt/decrypt operation.  As this
> driver does not seem to do that, it means that using it for more
> than one operation is broken.
>
> Fixes: eaed71a44ad9 ("crypto: caam - add ecb(*) support")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>

Acked-by: Ard Biesheuvel <ardb@kernel.org>

All internal users of ecb(arc4) use sync skciphers, so this should
only affect user space.

I do wonder if the others are doing any better - n2 and bcm iproc also
appear to keep the state in the TFM object, while I'd expect the
setkey() to be a simple memcpy(), and the initial state derivation to
be part of the encrypt flow, right?

Maybe we should add a test for this to tcrypt, i.e., do setkey() once
and do two encryptions of the same input, and check whether we get
back the original data.


> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index b2f9882bc010f..797bff9b93182 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -810,12 +810,6 @@ static int ctr_skcipher_setkey(struct crypto_skcipher *skcipher,
>         return skcipher_setkey(skcipher, key, keylen, ctx1_iv_off);
>  }
>
> -static int arc4_skcipher_setkey(struct crypto_skcipher *skcipher,
> -                               const u8 *key, unsigned int keylen)
> -{
> -       return skcipher_setkey(skcipher, key, keylen, 0);
> -}
> -
>  static int des_skcipher_setkey(struct crypto_skcipher *skcipher,
>                                const u8 *key, unsigned int keylen)
>  {
> @@ -1967,21 +1961,6 @@ static struct caam_skcipher_alg driver_algs[] = {
>                 },
>                 .caam.class1_alg_type = OP_ALG_ALGSEL_3DES | OP_ALG_AAI_ECB,
>         },
> -       {
> -               .skcipher = {
> -                       .base = {
> -                               .cra_name = "ecb(arc4)",
> -                               .cra_driver_name = "ecb-arc4-caam",
> -                               .cra_blocksize = ARC4_BLOCK_SIZE,
> -                       },
> -                       .setkey = arc4_skcipher_setkey,
> -                       .encrypt = skcipher_encrypt,
> -                       .decrypt = skcipher_decrypt,
> -                       .min_keysize = ARC4_MIN_KEY_SIZE,
> -                       .max_keysize = ARC4_MAX_KEY_SIZE,
> -               },
> -               .caam.class1_alg_type = OP_ALG_ALGSEL_ARC4 | OP_ALG_AAI_ECB,
> -       },
>  };
>
>  static struct caam_aead_alg driver_aeads[] = {
> @@ -3457,7 +3436,6 @@ int caam_algapi_init(struct device *ctrldev)
>         struct caam_drv_private *priv = dev_get_drvdata(ctrldev);
>         int i = 0, err = 0;
>         u32 aes_vid, aes_inst, des_inst, md_vid, md_inst, ccha_inst, ptha_inst;
> -       u32 arc4_inst;
>         unsigned int md_limit = SHA512_DIGEST_SIZE;
>         bool registered = false, gcm_support;
>
> @@ -3477,8 +3455,6 @@ int caam_algapi_init(struct device *ctrldev)
>                            CHA_ID_LS_DES_SHIFT;
>                 aes_inst = cha_inst & CHA_ID_LS_AES_MASK;
>                 md_inst = (cha_inst & CHA_ID_LS_MD_MASK) >> CHA_ID_LS_MD_SHIFT;
> -               arc4_inst = (cha_inst & CHA_ID_LS_ARC4_MASK) >>
> -                           CHA_ID_LS_ARC4_SHIFT;
>                 ccha_inst = 0;
>                 ptha_inst = 0;
>
> @@ -3499,7 +3475,6 @@ int caam_algapi_init(struct device *ctrldev)
>                 md_inst = mdha & CHA_VER_NUM_MASK;
>                 ccha_inst = rd_reg32(&priv->ctrl->vreg.ccha) & CHA_VER_NUM_MASK;
>                 ptha_inst = rd_reg32(&priv->ctrl->vreg.ptha) & CHA_VER_NUM_MASK;
> -               arc4_inst = rd_reg32(&priv->ctrl->vreg.afha) & CHA_VER_NUM_MASK;
>
>                 gcm_support = aesa & CHA_VER_MISC_AES_GCM;
>         }
> @@ -3522,10 +3497,6 @@ int caam_algapi_init(struct device *ctrldev)
>                 if (!aes_inst && (alg_sel == OP_ALG_ALGSEL_AES))
>                                 continue;
>
> -               /* Skip ARC4 algorithms if not supported by device */
> -               if (!arc4_inst && alg_sel == OP_ALG_ALGSEL_ARC4)
> -                       continue;
> -
>                 /*
>                  * Check support for AES modes not available
>                  * on LP devices.
> diff --git a/drivers/crypto/caam/compat.h b/drivers/crypto/caam/compat.h
> index 60e2a54c19f11..c3c22a8de4c00 100644
> --- a/drivers/crypto/caam/compat.h
> +++ b/drivers/crypto/caam/compat.h
> @@ -43,7 +43,6 @@
>  #include <crypto/akcipher.h>
>  #include <crypto/scatterwalk.h>
>  #include <crypto/skcipher.h>
> -#include <crypto/arc4.h>
>  #include <crypto/internal/skcipher.h>
>  #include <crypto/internal/hash.h>
>  #include <crypto/internal/rsa.h>
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel]

On Thu, 2 Jul 2020 at 09:27, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 2 Jul 2020 at 06:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > The arc4 algorithm requires storing state in the request context
> > in order to allow more than one encrypt/decrypt operation.  As this
> > driver does not seem to do that, it means that using it for more
> > than one operation is broken.
> >
> > Fixes: eaed71a44ad9 ("crypto: caam - add ecb(*) support")
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> >
>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>
> All internal users of ecb(arc4) use sync skciphers, so this should
> only affect user space.
>
> I do wonder if the others are doing any better - n2 and bcm iproc also
> appear to keep the state in the TFM object, while I'd expect the
> setkey() to be a simple memcpy(), and the initial state derivation to
> be part of the encrypt flow, right?
>
> Maybe we should add a test for this to tcrypt, i.e., do setkey() once
> and do two encryptions of the same input, and check whether we get
> back the original data.
>

Actually, it seems the generic ecb(arc4) is broken as well in this regard.

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel]

On Thu, 2 Jul 2020 at 09:32, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 2 Jul 2020 at 09:27, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 2 Jul 2020 at 06:36, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >
> > > The arc4 algorithm requires storing state in the request context
> > > in order to allow more than one encrypt/decrypt operation.  As this
> > > driver does not seem to do that, it means that using it for more
> > > than one operation is broken.
> > >
> > > Fixes: eaed71a44ad9 ("crypto: caam - add ecb(*) support")
> > > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> > >
> >
> > Acked-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > All internal users of ecb(arc4) use sync skciphers, so this should
> > only affect user space.
> >
> > I do wonder if the others are doing any better - n2 and bcm iproc also
> > appear to keep the state in the TFM object, while I'd expect the
> > setkey() to be a simple memcpy(), and the initial state derivation to
> > be part of the encrypt flow, right?
> >
> > Maybe we should add a test for this to tcrypt, i.e., do setkey() once
> > and do two encryptions of the same input, and check whether we get
> > back the original data.
> >
>
> Actually, it seems the generic ecb(arc4) is broken as well in this regard.

This may be strictly true, but perhaps reusing the key is not such a
great idea to begin with, given the lack of an IV, so the fact that
skcipher::setkey() operates on the TFM and not the request simply does
not match the ARC4 model.

I suppose you are looking into this for chaining algif_skipcher
requests, right? So in that case, the ARC4 state should really be
treated as an IV, which is owned by the caller, and not stored in
either the TFM or the skcipher request object.

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Herbert Xu]

On Thu, Jul 02, 2020 at 09:40:42AM +0200, Ard Biesheuvel wrote:
>
> I suppose you are looking into this for chaining algif_skipcher
> requests, right? So in that case, the ARC4 state should really be
> treated as an IV, which is owned by the caller, and not stored in
> either the TFM or the skcipher request object.

Yes I have considered this approach previously but it's just too
messy.  What I'm trying to do now is to allow the state to be stored
in the request object.  When combined with the proposed REQ_MORE
flag, this should be sufficient.  It evens works on XTS.

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

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel]

On Thu, 2 Jul 2020 at 09:45, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Jul 02, 2020 at 09:40:42AM +0200, Ard Biesheuvel wrote:
> >
> > I suppose you are looking into this for chaining algif_skipcher
> > requests, right? So in that case, the ARC4 state should really be
> > treated as an IV, which is owned by the caller, and not stored in
> > either the TFM or the skcipher request object.
>
> Yes I have considered this approach previously but it's just too
> messy.  What I'm trying to do now is to allow the state to be stored
> in the request object.  When combined with the proposed REQ_MORE
> flag, this should be sufficient.  It evens works on XTS.
>

But that requires the caller to reuse the same skcipher request object
when doing chaining, right?

Currently, we have no such requirement, and it would mean that the
request object's context struct should be aligned between different
implementations, e.g., when a driver ends up invoking a fallback for
the last N bytes of a chained request.

I'll wait for the code to be posted (please put me on cc), but my
suspicion is that carrying opaque state like that is going to bite us
down the road.

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Herbert Xu]

On Thu, Jul 02, 2020 at 09:51:29AM +0200, Ard Biesheuvel wrote:
>
> I'll wait for the code to be posted (please put me on cc), but my

Sure I will.

> suspicion is that carrying opaque state like that is going to bite us
> down the road.

Well it's only going to be arc4 at first, where it's definitely
an improvement over modifying the tfm context in encrypt/decrypt.

For XTS I haven't decided whether to go this way or not.  If it
does work out though we could even extend it to AEAD.

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

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Herbert Xu]

On Thu, Jul 02, 2020 at 05:56:16PM +1000, Herbert Xu wrote:
>
> For XTS I haven't decided whether to go this way or not.  If it
> does work out though we could even extend it to AEAD.

But there is clearly a need for this functionality, and it's
not just af_alg.  Have a look at net/sunrpc/auth_gss/gss_krb5_crypto.c,
it has three versions of the same crypto code (arc4, cts, and
everything else), in order to deal with continuing requests just
like algif_skcipher.

Perhaps at the end of this we could distill it down to just one.

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

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel]

On Thu, 2 Jul 2020 at 09:56, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Jul 02, 2020 at 09:51:29AM +0200, Ard Biesheuvel wrote:
> >
> > I'll wait for the code to be posted (please put me on cc), but my
>
> Sure I will.
>
> > suspicion is that carrying opaque state like that is going to bite us
> > down the road.
>
> Well it's only going to be arc4 at first, where it's definitely
> an improvement over modifying the tfm context in encrypt/decrypt.
>

I agree that the current approach is flawed, but starting multiple
requests with the same state essentially comes down to IV reuse in a
stream cipher, which will cause it to fail catastrophically.
(ciphertext1 ^ ciphertext2 == plaintext1 ^ plaintext2)

I wonder if we should simply try to get rid of arc4 in the crypto API,
as you suggested. There are a couple of WEP implementations that could
be switched over to the library interface, and the KerberosV
implementation of RC4-HMAC(md5) was added for Windows 2000
compatibility based on RFC 4757 [0], which was deprecated by RFC 8429
[1], since Windows Domain Controllers running Windows Server 2008r2 or
later can use newer algorithms.

[0] https://tools.ietf.org/html/rfc4757
[1] https://tools.ietf.org/html/rfc8429


>
> On Thu, Jul 02, 2020 at 05:56:16PM +1000, Herbert Xu wrote:
> >
> > For XTS I haven't decided whether to go this way or not.  If it
> > does work out though we could even extend it to AEAD.
>
> But there is clearly a need for this functionality, and it's
> not just af_alg.  Have a look at net/sunrpc/auth_gss/gss_krb5_crypto.c,
> it has three versions of the same crypto code (arc4, cts, and
> everything else), in order to deal with continuing requests just
> like algif_skcipher.
>
> Perhaps at the end of this we could distill it down to just one.
>

I agree that there is a gap here.

Perhaps we can decouple ARC4 from the other cases? ARC4 is too quirky
and irrelevant to model this on top of, I think.

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Herbert Xu]

On Thu, Jul 02, 2020 at 09:32:32AM +0200, Ard Biesheuvel wrote:
>
> Actually, it seems the generic ecb(arc4) is broken as well in this regard.

Not in the same way.  The generic arc4 stores the state in the
tfm so it works until you have two requests that share the same
tfm.

Since we only have two users right now we can fix this by moving
the state into the request and I'm working on it.

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

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Herbert Xu]

On Thu, Jul 02, 2020 at 09:27:33AM +0200, Ard Biesheuvel wrote:
> 
> I do wonder if the others are doing any better - n2 and bcm iproc also
> appear to keep the state in the TFM object, while I'd expect the
> setkey() to be a simple memcpy(), and the initial state derivation to
> be part of the encrypt flow, right?

bcm at least appears to be doing the right thing, but of course
without the hardware or a reference manual I can't be sure.

David can probably tell us whether n2 is capable of updating the
state at ent->enc_key_addr after each arc4 operation.

> Maybe we should add a test for this to tcrypt, i.e., do setkey() once
> and do two encryptions of the same input, and check whether we get
> back the original data.

Yes we could certainly add an arc4-only test.  Alternatively we
could kill the only in-kernel user (auth_gss) and then try to
remove the arc4 interface completely (and hope that nobody complains :)

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

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Horia Geantă]

On 7/2/2020 7:36 AM, Herbert Xu wrote:
> The arc4 algorithm requires storing state in the request context
> in order to allow more than one encrypt/decrypt operation.  As this
> driver does not seem to do that, it means that using it for more
> than one operation is broken.
> 
The fact that smth. is broken doesn't necessarily means it has to be removed.

Looking at the HW capabilities, I am sure the implementation could be
modified to save/restore the internal state to/from the request context.

Anyhow I would like to know if only the correctness is being debated,
or this patch should be dealt with in the larger context of
removing crypto API based ecb(arc4) altogether:
[RFC PATCH 0/7] crypto: get rid of ecb(arc4)
https://lore.kernel.org/linux-crypto/20200702101947.682-1-ardb@kernel.org/

Thanks,
Horia

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel]

On Sun, 5 Jul 2020 at 22:11, Horia Geantă <horia.geanta@nxp.com> wrote:
>
> On 7/2/2020 7:36 AM, Herbert Xu wrote:
> > The arc4 algorithm requires storing state in the request context
> > in order to allow more than one encrypt/decrypt operation.  As this
> > driver does not seem to do that, it means that using it for more
> > than one operation is broken.
> >
> The fact that smth. is broken doesn't necessarily means it has to be removed.
>
> Looking at the HW capabilities, I am sure the implementation could be
> modified to save/restore the internal state to/from the request context.
>
> Anyhow I would like to know if only the correctness is being debated,
> or this patch should be dealt with in the larger context of
> removing crypto API based ecb(arc4) altogether:
> [RFC PATCH 0/7] crypto: get rid of ecb(arc4)
> https://lore.kernel.org/linux-crypto/20200702101947.682-1-ardb@kernel.org/
>

The problem with 'fixing' ecb(arc4) is that it will make it less
secure than it already is. For instance, imagine two peers using the
generic ecb(arc4) implementation, and using a pair of skcipher TFMs to
en/decrypt the communication between them, similar to how the WEP code
works today. if we 'fix' the implementation, every request will be
served from the same initial state (including the key), and therefore
reuse the same keystream, resulting in catastrophic failure. (Of
course, the code should set a different key for each request anyway,
but failure to do so does not result in the same security fail with
the current implementation)

So the problem is really that the lack of a key vs IV distinction in
ARC4 means that it does not fit the skcipher model cleanly, and
issuing more than a single request without an intermediate setkey()
operation should be forbidden in any case.

The reason I suggested removing it is that we really have no use for
it in the kernel, and the only known af_alg users are dubious as well,
and so we'd be much better off simply getting rid of ecb(arc4)
entirely, not because any of the implementations are flawed, but
simply because I don't think we should waste more time on it in
general.

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Horia Geantă]

On 7/6/2020 4:43 PM, Ard Biesheuvel wrote:
> On Sun, 5 Jul 2020 at 22:11, Horia Geantă <horia.geanta@nxp.com> wrote:
>>
>> On 7/2/2020 7:36 AM, Herbert Xu wrote:
>>> The arc4 algorithm requires storing state in the request context
>>> in order to allow more than one encrypt/decrypt operation.  As this
>>> driver does not seem to do that, it means that using it for more
>>> than one operation is broken.
>>>
>> The fact that smth. is broken doesn't necessarily means it has to be removed.
>>
>> Looking at the HW capabilities, I am sure the implementation could be
>> modified to save/restore the internal state to/from the request context.
>>
>> Anyhow I would like to know if only the correctness is being debated,
>> or this patch should be dealt with in the larger context of
>> removing crypto API based ecb(arc4) altogether:
>> [RFC PATCH 0/7] crypto: get rid of ecb(arc4)
>> https://lore.kernel.org/linux-crypto/20200702101947.682-1-ardb@kernel.org/
>>
> 
> The problem with 'fixing' ecb(arc4) is that it will make it less
> secure than it already is. For instance, imagine two peers using the
> generic ecb(arc4) implementation, and using a pair of skcipher TFMs to
> en/decrypt the communication between them, similar to how the WEP code
> works today. if we 'fix' the implementation, every request will be
> served from the same initial state (including the key), and therefore
> reuse the same keystream, resulting in catastrophic failure. (Of
> course, the code should set a different key for each request anyway,
> but failure to do so does not result in the same security fail with
> the current implementation)
> 
> So the problem is really that the lack of a key vs IV distinction in
> ARC4 means that it does not fit the skcipher model cleanly, and
> issuing more than a single request without an intermediate setkey()
> operation should be forbidden in any case.
> 
> The reason I suggested removing it is that we really have no use for
> it in the kernel, and the only known af_alg users are dubious as well,
> and so we'd be much better off simply getting rid of ecb(arc4)
> entirely, not because any of the implementations are flawed, but
> simply because I don't think we should waste more time on it in
> general.
> 
Thanks Ard.
My understanding was along these lines, just wanted to make sure.

I think the commit message should be updated to reflect this logic:
indeed, caam's implementation of ecb(arc4) is broken,
but instead of fixing it, crypto API-based ecb(arc4)
is removed completely from the kernel (hence from caam driver)
due to skcipher limitations in terms of handling the keystream.

Horia

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Herbert Xu]

On Wed, Jul 08, 2020 at 07:24:08PM +0300, Horia Geantă wrote:
> 
> I think the commit message should be updated to reflect this logic:
> indeed, caam's implementation of ecb(arc4) is broken,
> but instead of fixing it, crypto API-based ecb(arc4)
> is removed completely from the kernel (hence from caam driver)
> due to skcipher limitations in terms of handling the keystream.

Actually that's not quite true.  The reason I create this patch
in the first place is to remove this limitation from skcipher.

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

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Horia Geantă]

On 7/9/2020 3:47 AM, Herbert Xu wrote:
> On Wed, Jul 08, 2020 at 07:24:08PM +0300, Horia Geantă wrote:
>>
>> I think the commit message should be updated to reflect this logic:
>> indeed, caam's implementation of ecb(arc4) is broken,
>> but instead of fixing it, crypto API-based ecb(arc4)
>> is removed completely from the kernel (hence from caam driver)
>> due to skcipher limitations in terms of handling the keystream.
> 
> Actually that's not quite true.  The reason I create this patch
> in the first place is to remove this limitation from skcipher.
> 
But the reason / context has changed in the meantime right?

If skcipher limitation is eliminated,
will it be possible to add ecb(arc4) implementation back in caam,
this time with the state stored in the request object?

My understanding is: no, if Ard's arc4 RFC series is merged.

Thanks,
Horia

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Ard Biesheuvel]

On Thu, 9 Jul 2020 at 11:53, Horia Geantă <horia.geanta@nxp.com> wrote:
>
> On 7/9/2020 3:47 AM, Herbert Xu wrote:
> > On Wed, Jul 08, 2020 at 07:24:08PM +0300, Horia Geantă wrote:
> >>
> >> I think the commit message should be updated to reflect this logic:
> >> indeed, caam's implementation of ecb(arc4) is broken,
> >> but instead of fixing it, crypto API-based ecb(arc4)
> >> is removed completely from the kernel (hence from caam driver)
> >> due to skcipher limitations in terms of handling the keystream.
> >
> > Actually that's not quite true.  The reason I create this patch
> > in the first place is to remove this limitation from skcipher.
> >
> But the reason / context has changed in the meantime right?
>
> If skcipher limitation is eliminated,
> will it be possible to add ecb(arc4) implementation back in caam,
> this time with the state stored in the request object?
>
> My understanding is: no, if Ard's arc4 RFC series is merged.
>

I would like the skcipher chaining discussion to focus on relevant and
typical skciphers like XTS and CBC-CTS, which is already tricky enough
to get right. As 'fixing' ecb(arc4) may open up security holes (given
that leaving the TFM version of the key untouched makes it more likely
that it gets reused inadvertently), I would prefer to get rid of it
entirely.

And what is the point of accelerated ecb(arc4) anyway? The internal
users all require sync skciphers (and those WEP/WPA fallbacks are
rarely used in practice, given that only ancient Wifi hardware relies
on them). We did identify a AF_ALG skcipher user that will be better
off using a C implementation in userland. And using RC4 for TLS was
explicitly forbidden by RFC 7465 5 years ago ...

Re: [PATCH] crypto: caam - Remove broken arc4 support

[Horia Geantă]

On 7/2/2020 7:36 AM, Herbert Xu wrote:
> The arc4 algorithm requires storing state in the request context
> in order to allow more than one encrypt/decrypt operation.  As this
> driver does not seem to do that, it means that using it for more
> than one operation is broken.
> 
> Fixes: eaed71a44ad9 ("crypto: caam - add ecb(*) support")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Given the circumstances, it's not feasible to fix arc4 in caam.

Link: https://lore.kernel.org/linux-crypto/CAMj1kXGvMe_A_iQ43Pmygg9xaAM-RLy=_M=v+eg--8xNmv9P+w@mail.gmail.com
Link: https://lore.kernel.org/linux-crypto/20200702101947.682-1-ardb@kernel.org
Acked-by: Horia Geantă <horia.geanta@nxp.com>

Horia