[RFC PATCH 0/3] move WEP implementation to skcipher interface

[RFC PATCH 0/3] move WEP implementation to skcipher interface

[Ard Biesheuvel]

One of the issues that I would like to see addressed in the crypto API
is they way the cipher abstraction is used. In general, a cipher should
never be used directly, and so it would be much better to clean up the
existing uses of ciphers outside of the crypto subsystem itself, so that
we can make the cipher abstraction part of the internal API, only to
be used by templates or crypto drivers that require them as a callback.

As a first step, this series moves all users of the 'arc4' cipher to
the ecb(arc4) skcipher, which happens to be implemented by the same
driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
actually evaluates to 1.

Next step would be to switch the users of the 'des' and 'aes' ciphers
to other interfaces that are more appropriate, either ecb(...) or a
library interface, which may be more appropriate in some cases. In any
case, the end result should be that ciphers are no longer used outside
of crypto/ and drivers/crypto/

This series is presented as an RFC, since I am mostly interested in
discussing the above, but I prefer to do so in the context of actual
patches rather than an abstract discussion.

Ard Biesheuvel (3):
  net/mac80211: switch to skcipher interface for arc4
  lib80211/tkip: switch to skcipher interface for arc4
  lib80211/wep: switch to skcipher interface for arc4

 net/mac80211/ieee80211_i.h         |  6 +-
 net/mac80211/key.h                 |  1 +
 net/mac80211/tkip.c                |  8 +-
 net/mac80211/tkip.h                |  4 +-
 net/mac80211/wep.c                 | 81 +++++++++++++++-----
 net/mac80211/wep.h                 |  4 +-
 net/mac80211/wpa.c                 |  4 +-
 net/wireless/lib80211_crypt_tkip.c | 61 ++++++++++-----
 net/wireless/lib80211_crypt_wep.c  | 52 +++++++++----
 9 files changed, 153 insertions(+), 68 deletions(-)

-- 
2.20.1

[RFC PATCH 1/3] net/mac80211: switch to skcipher interface for arc4

[Ard Biesheuvel]

The mac80211 WEP code uses the cipher interface, and invokes it byte
by byte. This is rather inefficient, and since the cipher interface in
general is not really suitable for use outside of the crypto subsystem,
move this code to use the skcipher interface instead.

Given that the only driver that provides a RC4 cipher also provides
a skcipher implementation, this is guaranteed to work. (Note that
ARC4_BLOCK_SIZE is #define'd as 1 so ecb(arc4) behaves as a stream
cipher, as is required in this case)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 net/mac80211/ieee80211_i.h |  6 +-
 net/mac80211/key.h         |  1 +
 net/mac80211/tkip.c        |  8 +-
 net/mac80211/tkip.h        |  4 +-
 net/mac80211/wep.c         | 81 +++++++++++++++-----
 net/mac80211/wep.h         |  4 +-
 net/mac80211/wpa.c         |  4 +-
 7 files changed, 75 insertions(+), 33 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 073a8235ae1b..c0773cf685b6 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1258,8 +1258,10 @@ struct ieee80211_local {
 
 	struct rate_control_ref *rate_ctrl;
 
-	struct crypto_cipher *wep_tx_tfm;
-	struct crypto_cipher *wep_rx_tfm;
+	struct crypto_skcipher *wep_tx_tfm;
+	struct crypto_skcipher *wep_rx_tfm;
+	struct skcipher_request *wep_tx_req;
+	struct skcipher_request *wep_rx_req;
 	u32 wep_iv;
 
 	/* see iface.c */
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index f06fbd03d235..ee5e7f30a1f1 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -13,6 +13,7 @@
 #include <linux/types.h>
 #include <linux/list.h>
 #include <linux/crypto.h>
+#include <crypto/skcipher.h>
 #include <linux/rcupdate.h>
 #include <net/mac80211.h>
 
diff --git a/net/mac80211/tkip.c b/net/mac80211/tkip.c
index b3622823bad2..23dd962633fe 100644
--- a/net/mac80211/tkip.c
+++ b/net/mac80211/tkip.c
@@ -222,7 +222,7 @@ EXPORT_SYMBOL(ieee80211_get_tkip_p2k);
  * @payload_len is the length of payload (_not_ including IV/ICV length).
  * @ta is the transmitter addresses.
  */
-int ieee80211_tkip_encrypt_data(struct crypto_cipher *tfm,
+int ieee80211_tkip_encrypt_data(struct skcipher_request *req,
 				struct ieee80211_key *key,
 				struct sk_buff *skb,
 				u8 *payload, size_t payload_len)
@@ -231,7 +231,7 @@ int ieee80211_tkip_encrypt_data(struct crypto_cipher *tfm,
 
 	ieee80211_get_tkip_p2k(&key->conf, skb, rc4key);
 
-	return ieee80211_wep_encrypt_data(tfm, rc4key, 16,
+	return ieee80211_wep_encrypt_data(req, rc4key, 16,
 					  payload, payload_len);
 }
 
@@ -239,7 +239,7 @@ int ieee80211_tkip_encrypt_data(struct crypto_cipher *tfm,
  * beginning of the buffer containing IEEE 802.11 header payload, i.e.,
  * including IV, Ext. IV, real data, Michael MIC, ICV. @payload_len is the
  * length of payload, including IV, Ext. IV, MIC, ICV.  */
-int ieee80211_tkip_decrypt_data(struct crypto_cipher *tfm,
+int ieee80211_tkip_decrypt_data(struct skcipher_request *req,
 				struct ieee80211_key *key,
 				u8 *payload, size_t payload_len, u8 *ta,
 				u8 *ra, int only_iv, int queue,
@@ -297,7 +297,7 @@ int ieee80211_tkip_decrypt_data(struct crypto_cipher *tfm,
 
 	tkip_mixing_phase2(tk, &rx_ctx->ctx, iv16, rc4key);
 
-	res = ieee80211_wep_decrypt_data(tfm, rc4key, 16, pos, payload_len - 12);
+	res = ieee80211_wep_decrypt_data(req, rc4key, 16, pos, payload_len - 12);
  done:
 	if (res == TKIP_DECRYPT_OK) {
 		/*
diff --git a/net/mac80211/tkip.h b/net/mac80211/tkip.h
index a1bcbfbefe7c..e66aac448786 100644
--- a/net/mac80211/tkip.h
+++ b/net/mac80211/tkip.h
@@ -13,7 +13,7 @@
 #include <linux/crypto.h>
 #include "key.h"
 
-int ieee80211_tkip_encrypt_data(struct crypto_cipher *tfm,
+int ieee80211_tkip_encrypt_data(struct skcipher_request *req,
 				struct ieee80211_key *key,
 				struct sk_buff *skb,
 				u8 *payload, size_t payload_len);
@@ -24,7 +24,7 @@ enum {
 	TKIP_DECRYPT_INVALID_KEYIDX = -2,
 	TKIP_DECRYPT_REPLAY = -3,
 };
-int ieee80211_tkip_decrypt_data(struct crypto_cipher *tfm,
+int ieee80211_tkip_decrypt_data(struct skcipher_request *req,
 				struct ieee80211_key *key,
 				u8 *payload, size_t payload_len, u8 *ta,
 				u8 *ra, int only_iv, int queue,
diff --git a/net/mac80211/wep.c b/net/mac80211/wep.c
index bfe9ed9f4c48..0336afb70c2b 100644
--- a/net/mac80211/wep.c
+++ b/net/mac80211/wep.c
@@ -27,31 +27,64 @@
 
 int ieee80211_wep_init(struct ieee80211_local *local)
 {
+	int err;
+
 	/* start WEP IV from a random value */
 	get_random_bytes(&local->wep_iv, IEEE80211_WEP_IV_LEN);
 
-	local->wep_tx_tfm = crypto_alloc_cipher("arc4", 0, 0);
+	local->wep_tx_tfm = crypto_alloc_skcipher("ecb(arc4)", 0,
+						  CRYPTO_ALG_ASYNC);
 	if (IS_ERR(local->wep_tx_tfm)) {
 		local->wep_rx_tfm = ERR_PTR(-EINVAL);
 		return PTR_ERR(local->wep_tx_tfm);
 	}
 
-	local->wep_rx_tfm = crypto_alloc_cipher("arc4", 0, 0);
+	local->wep_rx_tfm = crypto_alloc_skcipher("ecb(arc4)", 0,
+						  CRYPTO_ALG_ASYNC);
 	if (IS_ERR(local->wep_rx_tfm)) {
-		crypto_free_cipher(local->wep_tx_tfm);
-		local->wep_tx_tfm = ERR_PTR(-EINVAL);
-		return PTR_ERR(local->wep_rx_tfm);
+		err = PTR_ERR(local->wep_rx_tfm);
+		goto free_tx_tfm;
+	}
+
+	local->wep_tx_req = skcipher_request_alloc(local->wep_tx_tfm,
+						   GFP_KERNEL);
+	if (!local->wep_tx_req) {
+		err = -ENOMEM;
+		goto free_rx_tfm;
+	}
+
+	local->wep_rx_req = skcipher_request_alloc(local->wep_rx_tfm,
+						   GFP_KERNEL);
+	if (!local->wep_rx_req) {
+		err = -ENOMEM;
+		goto free_tx_req;
 	}
 
 	return 0;
+
+free_tx_req:
+	skcipher_request_free(local->wep_tx_req);
+	local->wep_tx_req = NULL;
+free_rx_tfm:
+	crypto_free_skcipher(local->wep_rx_tfm);
+	local->wep_rx_tfm = ERR_PTR(-EINVAL);
+free_tx_tfm:
+	crypto_free_skcipher(local->wep_tx_tfm);
+	local->wep_tx_tfm = ERR_PTR(-EINVAL);
+
+	return err;
 }
 
 void ieee80211_wep_free(struct ieee80211_local *local)
 {
+	if (local->wep_tx_req)
+		skcipher_request_free(local->wep_tx_req);
+	if (local->wep_rx_req)
+		skcipher_request_free(local->wep_rx_req);
 	if (!IS_ERR(local->wep_tx_tfm))
-		crypto_free_cipher(local->wep_tx_tfm);
+		crypto_free_skcipher(local->wep_tx_tfm);
 	if (!IS_ERR(local->wep_rx_tfm))
-		crypto_free_cipher(local->wep_rx_tfm);
+		crypto_free_skcipher(local->wep_rx_tfm);
 }
 
 static inline bool ieee80211_wep_weak_iv(u32 iv, int keylen)
@@ -131,21 +164,24 @@ static void ieee80211_wep_remove_iv(struct ieee80211_local *local,
 /* Perform WEP encryption using given key. data buffer must have tailroom
  * for 4-byte ICV. data_len must not include this ICV. Note: this function
  * does _not_ add IV. data = RC4(data | CRC32(data)) */
-int ieee80211_wep_encrypt_data(struct crypto_cipher *tfm, u8 *rc4key,
+int ieee80211_wep_encrypt_data(struct skcipher_request *req, u8 *rc4key,
 			       size_t klen, u8 *data, size_t data_len)
 {
+	struct scatterlist sg;
 	__le32 icv;
-	int i;
 
-	if (IS_ERR(tfm))
+	if (!req)
 		return -1;
 
 	icv = cpu_to_le32(~crc32_le(~0, data, data_len));
 	put_unaligned(icv, (__le32 *)(data + data_len));
 
-	crypto_cipher_setkey(tfm, rc4key, klen);
-	for (i = 0; i < data_len + IEEE80211_WEP_ICV_LEN; i++)
-		crypto_cipher_encrypt_one(tfm, data + i, data + i);
+	crypto_skcipher_setkey(crypto_skcipher_reqtfm(req), rc4key, klen);
+	sg_init_one(&sg, data, data_len + IEEE80211_WEP_ICV_LEN);
+	skcipher_request_set_crypt(req, &sg, &sg,
+				   data_len + IEEE80211_WEP_ICV_LEN, NULL);
+	if (crypto_skcipher_encrypt(req) < 0)
+		return -1;
 
 	return 0;
 }
@@ -184,7 +220,7 @@ int ieee80211_wep_encrypt(struct ieee80211_local *local,
 	/* Add room for ICV */
 	skb_put(skb, IEEE80211_WEP_ICV_LEN);
 
-	return ieee80211_wep_encrypt_data(local->wep_tx_tfm, rc4key, keylen + 3,
+	return ieee80211_wep_encrypt_data(local->wep_tx_req, rc4key, keylen + 3,
 					  iv + IEEE80211_WEP_IV_LEN, len);
 }
 
@@ -192,18 +228,21 @@ int ieee80211_wep_encrypt(struct ieee80211_local *local,
 /* Perform WEP decryption using given key. data buffer includes encrypted
  * payload, including 4-byte ICV, but _not_ IV. data_len must not include ICV.
  * Return 0 on success and -1 on ICV mismatch. */
-int ieee80211_wep_decrypt_data(struct crypto_cipher *tfm, u8 *rc4key,
+int ieee80211_wep_decrypt_data(struct skcipher_request *req, u8 *rc4key,
 			       size_t klen, u8 *data, size_t data_len)
 {
+	struct scatterlist sg;
 	__le32 crc;
-	int i;
 
-	if (IS_ERR(tfm))
+	if (!req)
 		return -1;
 
-	crypto_cipher_setkey(tfm, rc4key, klen);
-	for (i = 0; i < data_len + IEEE80211_WEP_ICV_LEN; i++)
-		crypto_cipher_decrypt_one(tfm, data + i, data + i);
+	crypto_skcipher_setkey(crypto_skcipher_reqtfm(req), rc4key, klen);
+	sg_init_one(&sg, data, data_len + IEEE80211_WEP_ICV_LEN);
+	skcipher_request_set_crypt(req, &sg, &sg,
+				   data_len + IEEE80211_WEP_ICV_LEN, NULL);
+	if (crypto_skcipher_decrypt(req) < 0)
+		return -1;
 
 	crc = cpu_to_le32(~crc32_le(~0, data, data_len));
 	if (memcmp(&crc, data + data_len, IEEE80211_WEP_ICV_LEN) != 0)
@@ -256,7 +295,7 @@ static int ieee80211_wep_decrypt(struct ieee80211_local *local,
 	/* Copy rest of the WEP key (the secret part) */
 	memcpy(rc4key + 3, key->conf.key, key->conf.keylen);
 
-	if (ieee80211_wep_decrypt_data(local->wep_rx_tfm, rc4key, klen,
+	if (ieee80211_wep_decrypt_data(local->wep_rx_req, rc4key, klen,
 				       skb->data + hdrlen +
 				       IEEE80211_WEP_IV_LEN, len))
 		ret = -1;
diff --git a/net/mac80211/wep.h b/net/mac80211/wep.h
index 9615749d1f65..6a392b1ed4b7 100644
--- a/net/mac80211/wep.h
+++ b/net/mac80211/wep.h
@@ -18,12 +18,12 @@
 
 int ieee80211_wep_init(struct ieee80211_local *local);
 void ieee80211_wep_free(struct ieee80211_local *local);
-int ieee80211_wep_encrypt_data(struct crypto_cipher *tfm, u8 *rc4key,
+int ieee80211_wep_encrypt_data(struct skcipher_request *req, u8 *rc4key,
 				size_t klen, u8 *data, size_t data_len);
 int ieee80211_wep_encrypt(struct ieee80211_local *local,
 			  struct sk_buff *skb,
 			  const u8 *key, int keylen, int keyidx);
-int ieee80211_wep_decrypt_data(struct crypto_cipher *tfm, u8 *rc4key,
+int ieee80211_wep_decrypt_data(struct skcipher_request *req, u8 *rc4key,
 			       size_t klen, u8 *data, size_t data_len);
 
 ieee80211_rx_result
diff --git a/net/mac80211/wpa.c b/net/mac80211/wpa.c
index 58d0b258b684..a7870ec67a26 100644
--- a/net/mac80211/wpa.c
+++ b/net/mac80211/wpa.c
@@ -242,7 +242,7 @@ static int tkip_encrypt_skb(struct ieee80211_tx_data *tx, struct sk_buff *skb)
 	/* Add room for ICV */
 	skb_put(skb, IEEE80211_TKIP_ICV_LEN);
 
-	return ieee80211_tkip_encrypt_data(tx->local->wep_tx_tfm,
+	return ieee80211_tkip_encrypt_data(tx->local->wep_tx_req,
 					   key, skb, pos, len);
 }
 
@@ -293,7 +293,7 @@ ieee80211_crypto_tkip_decrypt(struct ieee80211_rx_data *rx)
 	if (status->flag & RX_FLAG_DECRYPTED)
 		hwaccel = 1;
 
-	res = ieee80211_tkip_decrypt_data(rx->local->wep_rx_tfm,
+	res = ieee80211_tkip_decrypt_data(rx->local->wep_rx_req,
 					  key, skb->data + hdrlen,
 					  skb->len - hdrlen, rx->sta->sta.addr,
 					  hdr->addr1, hwaccel, rx->security_idx,
-- 
2.20.1

[RFC PATCH 2/3] lib80211/tkip: switch to skcipher interface for arc4

[Ard Biesheuvel]

The TKIP code uses the arc4 cipher interface, and invokes it byte by
byte. This is rather inefficient, and since the cipher interface in
general is not really suitable for use outside of the crypto subsystem,
move this code to use the skcipher interface instead.

Given that the only driver that provides a RC4 cipher also provides
a skcipher implementation, this is guaranteed to work. (Note that
ARC4_BLOCK_SIZE is #define'd as 1 so ecb(arc4) behaves as a stream
cipher, as is required in this case)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 net/wireless/lib80211_crypt_tkip.c | 61 ++++++++++++++------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/net/wireless/lib80211_crypt_tkip.c b/net/wireless/lib80211_crypt_tkip.c
index 11eaa5956f00..456c85b03b32 100644
--- a/net/wireless/lib80211_crypt_tkip.c
+++ b/net/wireless/lib80211_crypt_tkip.c
@@ -30,6 +30,7 @@
 #include <net/iw_handler.h>
 
 #include <crypto/hash.h>
+#include <crypto/skcipher.h>
 #include <linux/crypto.h>
 #include <linux/crc32.h>
 
@@ -64,9 +65,11 @@ struct lib80211_tkip_data {
 
 	int key_idx;
 
-	struct crypto_cipher *rx_tfm_arc4;
+	struct crypto_skcipher *rx_tfm_arc4;
+	struct skcipher_request *rx_req_arc4;
 	struct crypto_shash *rx_tfm_michael;
-	struct crypto_cipher *tx_tfm_arc4;
+	struct crypto_skcipher *tx_tfm_arc4;
+	struct skcipher_request *tx_req_arc4;
 	struct crypto_shash *tx_tfm_michael;
 
 	/* scratch buffers for virt_to_page() (crypto API) */
@@ -99,7 +102,8 @@ static void *lib80211_tkip_init(int key_idx)
 
 	priv->key_idx = key_idx;
 
-	priv->tx_tfm_arc4 = crypto_alloc_cipher("arc4", 0, 0);
+	priv->tx_tfm_arc4 = crypto_alloc_skcipher("ecb(arc4)", 0,
+						  CRYPTO_ALG_ASYNC);
 	if (IS_ERR(priv->tx_tfm_arc4)) {
 		priv->tx_tfm_arc4 = NULL;
 		goto fail;
@@ -111,7 +115,8 @@ static void *lib80211_tkip_init(int key_idx)
 		goto fail;
 	}
 
-	priv->rx_tfm_arc4 = crypto_alloc_cipher("arc4", 0, 0);
+	priv->rx_tfm_arc4 = crypto_alloc_skcipher("ecb(arc4)", 0,
+						  CRYPTO_ALG_ASYNC);
 	if (IS_ERR(priv->rx_tfm_arc4)) {
 		priv->rx_tfm_arc4 = NULL;
 		goto fail;
@@ -123,14 +128,26 @@ static void *lib80211_tkip_init(int key_idx)
 		goto fail;
 	}
 
+	priv->tx_req_arc4 = skcipher_request_alloc(priv->tx_tfm_arc4,
+						   GFP_KERNEL);
+	if (!priv->tx_req_arc4)
+		goto fail;
+
+	priv->rx_req_arc4 = skcipher_request_alloc(priv->rx_tfm_arc4,
+						   GFP_KERNEL);
+	if (!priv->rx_req_arc4)
+		goto fail;
+
 	return priv;
 
       fail:
 	if (priv) {
+		skcipher_request_free(priv->tx_req_arc4);
+		skcipher_request_free(priv->rx_req_arc4);
 		crypto_free_shash(priv->tx_tfm_michael);
-		crypto_free_cipher(priv->tx_tfm_arc4);
+		crypto_free_skcipher(priv->tx_tfm_arc4);
 		crypto_free_shash(priv->rx_tfm_michael);
-		crypto_free_cipher(priv->rx_tfm_arc4);
+		crypto_free_skcipher(priv->rx_tfm_arc4);
 		kfree(priv);
 	}
 
@@ -141,10 +158,12 @@ static void lib80211_tkip_deinit(void *priv)
 {
 	struct lib80211_tkip_data *_priv = priv;
 	if (_priv) {
+		skcipher_request_free(_priv->tx_req_arc4);
+		skcipher_request_free(_priv->rx_req_arc4);
 		crypto_free_shash(_priv->tx_tfm_michael);
-		crypto_free_cipher(_priv->tx_tfm_arc4);
+		crypto_free_skcipher(_priv->tx_tfm_arc4);
 		crypto_free_shash(_priv->rx_tfm_michael);
-		crypto_free_cipher(_priv->rx_tfm_arc4);
+		crypto_free_skcipher(_priv->rx_tfm_arc4);
 	}
 	kfree(priv);
 }
@@ -342,10 +361,10 @@ static int lib80211_tkip_hdr(struct sk_buff *skb, int hdr_len,
 static int lib80211_tkip_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
 {
 	struct lib80211_tkip_data *tkey = priv;
+	struct scatterlist sg;
 	int len;
 	u8 rc4key[16], *pos, *icv;
 	u32 crc;
-	int i;
 
 	if (tkey->flags & IEEE80211_CRYPTO_TKIP_COUNTERMEASURES) {
 		struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
@@ -370,10 +389,11 @@ static int lib80211_tkip_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
 	icv[2] = crc >> 16;
 	icv[3] = crc >> 24;
 
-	crypto_cipher_setkey(tkey->tx_tfm_arc4, rc4key, 16);
-	for (i = 0; i < len + 4; i++)
-		crypto_cipher_encrypt_one(tkey->tx_tfm_arc4, pos + i, pos + i);
-	return 0;
+	crypto_skcipher_setkey(tkey->tx_tfm_arc4, rc4key, 16);
+	sg_init_one(&sg, pos, len + 4);
+	skcipher_request_set_crypt(tkey->tx_req_arc4, &sg, &sg, len + 4, NULL);
+
+	return crypto_skcipher_encrypt(tkey->tx_req_arc4);
 }
 
 /*
@@ -392,6 +412,7 @@ static inline int tkip_replay_check(u32 iv32_n, u16 iv16_n,
 static int lib80211_tkip_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
 {
 	struct lib80211_tkip_data *tkey = priv;
+	struct scatterlist sg;
 	u8 rc4key[16];
 	u8 keyidx, *pos;
 	u32 iv32;
@@ -400,7 +421,6 @@ static int lib80211_tkip_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
 	u8 icv[4];
 	u32 crc;
 	int plen;
-	int i;
 
 	hdr = (struct ieee80211_hdr *)skb->data;
 
@@ -453,9 +473,12 @@ static int lib80211_tkip_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
 
 	plen = skb->len - hdr_len - 12;
 
-	crypto_cipher_setkey(tkey->rx_tfm_arc4, rc4key, 16);
-	for (i = 0; i < plen + 4; i++)
-		crypto_cipher_decrypt_one(tkey->rx_tfm_arc4, pos + i, pos + i);
+	crypto_skcipher_setkey(tkey->rx_tfm_arc4, rc4key, 16);
+	sg_init_one(&sg, pos, plen + 4);
+	skcipher_request_set_crypt(tkey->rx_req_arc4, &sg, &sg, plen + 4, NULL);
+
+	if (crypto_skcipher_decrypt(tkey->rx_req_arc4))
+		return -7;
 
 	crc = ~crc32_le(~0, pos, plen);
 	icv[0] = crc;
@@ -640,9 +663,9 @@ static int lib80211_tkip_set_key(void *key, int len, u8 * seq, void *priv)
 	struct lib80211_tkip_data *tkey = priv;
 	int keyidx;
 	struct crypto_shash *tfm = tkey->tx_tfm_michael;
-	struct crypto_cipher *tfm2 = tkey->tx_tfm_arc4;
+	struct crypto_skcipher *tfm2 = tkey->tx_tfm_arc4;
 	struct crypto_shash *tfm3 = tkey->rx_tfm_michael;
-	struct crypto_cipher *tfm4 = tkey->rx_tfm_arc4;
+	struct crypto_skcipher *tfm4 = tkey->rx_tfm_arc4;
 
 	keyidx = tkey->key_idx;
 	memset(tkey, 0, sizeof(*tkey));
-- 
2.20.1

[RFC PATCH 3/3] lib80211/wep: switch to skcipher interface for arc4

[Ard Biesheuvel]

The WEP code uses the arc4 cipher interface, and invokes it byte by
byte. This is rather inefficient, and since the cipher interface in
general is not really suitable for use outside of the crypto subsystem,
move this code to use the skcipher interface instead.

Given that the only driver that provides a RC4 cipher also provides
a skcipher implementation, this is guaranteed to work. (Note that
ARC4_BLOCK_SIZE is #define'd as 1 so ecb(arc4) behaves as a stream
cipher, as is required in this case)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 net/wireless/lib80211_crypt_wep.c | 52 ++++++++++++++------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/net/wireless/lib80211_crypt_wep.c b/net/wireless/lib80211_crypt_wep.c
index 20c1ad63ad44..461f94afe6e9 100644
--- a/net/wireless/lib80211_crypt_wep.c
+++ b/net/wireless/lib80211_crypt_wep.c
@@ -23,6 +23,7 @@
 #include <net/lib80211.h>
 
 #include <linux/crypto.h>
+#include <crypto/skcipher.h>
 #include <linux/crc32.h>
 
 MODULE_AUTHOR("Jouni Malinen");
@@ -35,8 +36,10 @@ struct lib80211_wep_data {
 	u8 key[WEP_KEY_LEN + 1];
 	u8 key_len;
 	u8 key_idx;
-	struct crypto_cipher *tx_tfm;
-	struct crypto_cipher *rx_tfm;
+	struct crypto_skcipher *tx_tfm;
+	struct crypto_skcipher *rx_tfm;
+	struct skcipher_request *tx_req;
+	struct skcipher_request *rx_req;
 };
 
 static void *lib80211_wep_init(int keyidx)
@@ -48,17 +51,26 @@ static void *lib80211_wep_init(int keyidx)
 		goto fail;
 	priv->key_idx = keyidx;
 
-	priv->tx_tfm = crypto_alloc_cipher("arc4", 0, 0);
+	priv->tx_tfm = crypto_alloc_skcipher("ecb(arc4)", 0, 0);
 	if (IS_ERR(priv->tx_tfm)) {
 		priv->tx_tfm = NULL;
 		goto fail;
 	}
 
-	priv->rx_tfm = crypto_alloc_cipher("arc4", 0, 0);
+	priv->rx_tfm = crypto_alloc_skcipher("arc4", 0, 0);
 	if (IS_ERR(priv->rx_tfm)) {
 		priv->rx_tfm = NULL;
 		goto fail;
 	}
+
+	priv->tx_req = skcipher_request_alloc(priv->tx_tfm, GFP_KERNEL);
+	if (!priv->tx_req)
+		goto fail;
+
+	priv->rx_req = skcipher_request_alloc(priv->rx_tfm, GFP_KERNEL);
+	if (!priv->tx_req)
+		goto fail;
+
 	/* start WEP IV from a random value */
 	get_random_bytes(&priv->iv, 4);
 
@@ -66,8 +78,10 @@ static void *lib80211_wep_init(int keyidx)
 
       fail:
 	if (priv) {
-		crypto_free_cipher(priv->tx_tfm);
-		crypto_free_cipher(priv->rx_tfm);
+		skcipher_request_free(priv->tx_req);
+		skcipher_request_free(priv->rx_req);
+		crypto_free_skcipher(priv->tx_tfm);
+		crypto_free_skcipher(priv->rx_tfm);
 		kfree(priv);
 	}
 	return NULL;
@@ -77,8 +91,10 @@ static void lib80211_wep_deinit(void *priv)
 {
 	struct lib80211_wep_data *_priv = priv;
 	if (_priv) {
-		crypto_free_cipher(_priv->tx_tfm);
-		crypto_free_cipher(_priv->rx_tfm);
+		skcipher_request_free(_priv->tx_req);
+		skcipher_request_free(_priv->rx_req);
+		crypto_free_skcipher(_priv->tx_tfm);
+		crypto_free_skcipher(_priv->rx_tfm);
 	}
 	kfree(priv);
 }
@@ -129,10 +145,10 @@ static int lib80211_wep_build_iv(struct sk_buff *skb, int hdr_len,
 static int lib80211_wep_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
 {
 	struct lib80211_wep_data *wep = priv;
+	struct scatterlist sg;
 	u32 crc, klen, len;
 	u8 *pos, *icv;
 	u8 key[WEP_KEY_LEN + 3];
-	int i;
 
 	/* other checks are in lib80211_wep_build_iv */
 	if (skb_tailroom(skb) < 4)
@@ -160,10 +176,11 @@ static int lib80211_wep_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
 	icv[2] = crc >> 16;
 	icv[3] = crc >> 24;
 
-	crypto_cipher_setkey(wep->tx_tfm, key, klen);
-
-	for (i = 0; i < len + 4; i++)
-		crypto_cipher_encrypt_one(wep->tx_tfm, pos + i, pos + i);
+	crypto_skcipher_setkey(wep->tx_tfm, key, klen);
+	sg_init_one(&sg, pos, len + 4);
+	skcipher_request_set_crypt(wep->tx_req, &sg, &sg, len + 4, NULL);
+	if (crypto_skcipher_encrypt(wep->tx_req) < 0)
+		return -1;
 
 	return 0;
 }
@@ -178,6 +195,7 @@ static int lib80211_wep_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
 static int lib80211_wep_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
 {
 	struct lib80211_wep_data *wep = priv;
+	struct scatterlist sg;
 	u32 crc, klen, plen;
 	u8 key[WEP_KEY_LEN + 3];
 	u8 keyidx, *pos, icv[4];
@@ -202,9 +220,11 @@ static int lib80211_wep_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
 	/* Apply RC4 to data and compute CRC32 over decrypted data */
 	plen = skb->len - hdr_len - 8;
 
-	crypto_cipher_setkey(wep->rx_tfm, key, klen);
-	for (i = 0; i < plen + 4; i++)
-		crypto_cipher_decrypt_one(wep->rx_tfm, pos + i, pos + i);
+	crypto_skcipher_setkey(wep->rx_tfm, key, klen);
+	sg_init_one(&sg, pos, plen + 4);
+	skcipher_request_set_crypt(wep->rx_req, &sg, &sg, plen + 4, NULL);
+	if (crypto_skcipher_decrypt(wep->rx_req) < 0)
+		return -1;
 
 	crc = ~crc32_le(~0, pos, plen);
 	icv[0] = crc;
-- 
2.20.1

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Eric Biggers]

On Fri, Jun 07, 2019 at 04:49:41PM +0200, Ard Biesheuvel wrote:
> One of the issues that I would like to see addressed in the crypto API
> is they way the cipher abstraction is used. In general, a cipher should
> never be used directly, and so it would be much better to clean up the
> existing uses of ciphers outside of the crypto subsystem itself, so that
> we can make the cipher abstraction part of the internal API, only to
> be used by templates or crypto drivers that require them as a callback.
> 
> As a first step, this series moves all users of the 'arc4' cipher to
> the ecb(arc4) skcipher, which happens to be implemented by the same
> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> actually evaluates to 1.
> 
> Next step would be to switch the users of the 'des' and 'aes' ciphers
> to other interfaces that are more appropriate, either ecb(...) or a
> library interface, which may be more appropriate in some cases. In any
> case, the end result should be that ciphers are no longer used outside
> of crypto/ and drivers/crypto/
> 
> This series is presented as an RFC, since I am mostly interested in
> discussing the above, but I prefer to do so in the context of actual
> patches rather than an abstract discussion.
> 
> Ard Biesheuvel (3):
>   net/mac80211: switch to skcipher interface for arc4
>   lib80211/tkip: switch to skcipher interface for arc4
>   lib80211/wep: switch to skcipher interface for arc4
> 

The way the crypto API exposes ARC4 is definitely broken.  It treats it as a
block cipher (with a block size of 1 byte...), when it's actually a stream
cipher.  Also, it violates the API by modifying the key during each encryption.

Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
using, and the users call it on virtual addresses, perhaps we should instead
remove it from the crypto API and provide a library function arc4_crypt()?  We'd
lose support for ARC4 in three hardware drivers, but are there real users who
really are using ARC4 and need those to get acceptable performance?  Note that
they aren't being used in the cases where the 'cipher' API is currently being
used, so it would only be the current 'skcipher' users that might matter.

Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
seems unlikely...

As for removing the "cipher" API entirely, we'd have to consider how to convert
all the current users, not just ARC4, so that would be a somewhat different
discussion.  How do you propose to handle dm-crypt and fscrypt which use the
cipher API to do ESSIV?

- Eric

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Ard Biesheuvel]

On Fri, 7 Jun 2019 at 19:59, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Jun 07, 2019 at 04:49:41PM +0200, Ard Biesheuvel wrote:
> > One of the issues that I would like to see addressed in the crypto API
> > is they way the cipher abstraction is used. In general, a cipher should
> > never be used directly, and so it would be much better to clean up the
> > existing uses of ciphers outside of the crypto subsystem itself, so that
> > we can make the cipher abstraction part of the internal API, only to
> > be used by templates or crypto drivers that require them as a callback.
> >
> > As a first step, this series moves all users of the 'arc4' cipher to
> > the ecb(arc4) skcipher, which happens to be implemented by the same
> > driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> > actually evaluates to 1.
> >
> > Next step would be to switch the users of the 'des' and 'aes' ciphers
> > to other interfaces that are more appropriate, either ecb(...) or a
> > library interface, which may be more appropriate in some cases. In any
> > case, the end result should be that ciphers are no longer used outside
> > of crypto/ and drivers/crypto/
> >
> > This series is presented as an RFC, since I am mostly interested in
> > discussing the above, but I prefer to do so in the context of actual
> > patches rather than an abstract discussion.
> >
> > Ard Biesheuvel (3):
> >   net/mac80211: switch to skcipher interface for arc4
> >   lib80211/tkip: switch to skcipher interface for arc4
> >   lib80211/wep: switch to skcipher interface for arc4
> >
>
> The way the crypto API exposes ARC4 is definitely broken.  It treats it as a
> block cipher (with a block size of 1 byte...), when it's actually a stream
> cipher.  Also, it violates the API by modifying the key during each encryption.
>
> Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
> using, and the users call it on virtual addresses, perhaps we should instead
> remove it from the crypto API and provide a library function arc4_crypt()?  We'd
> lose support for ARC4 in three hardware drivers, but are there real users who
> really are using ARC4 and need those to get acceptable performance?  Note that
> they aren't being used in the cases where the 'cipher' API is currently being
> used, so it would only be the current 'skcipher' users that might matter.
>

In fact, this is what I started out doing, i.e., factor out the core
arc4 code into crypto/arc4_lib.c, and make the existing driver a thin
wrapper around it, so that we can invoke the library directly.

> Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
> seems unlikely...
>

Yes, that seems highly unlikely.

> As for removing the "cipher" API entirely, we'd have to consider how to convert
> all the current users, not just ARC4, so that would be a somewhat different
> discussion.  How do you propose to handle dm-crypt and fscrypt which use the
> cipher API to do ESSIV?
>

Without having looked in too much detail, ESSIV seems like something
that could be moved into the crypto subsystem, and be implemented as a
template.

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Marcel Holtmann]

Hi Eric,

>> One of the issues that I would like to see addressed in the crypto API
>> is they way the cipher abstraction is used. In general, a cipher should
>> never be used directly, and so it would be much better to clean up the
>> existing uses of ciphers outside of the crypto subsystem itself, so that
>> we can make the cipher abstraction part of the internal API, only to
>> be used by templates or crypto drivers that require them as a callback.
>> 
>> As a first step, this series moves all users of the 'arc4' cipher to
>> the ecb(arc4) skcipher, which happens to be implemented by the same
>> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
>> actually evaluates to 1.
>> 
>> Next step would be to switch the users of the 'des' and 'aes' ciphers
>> to other interfaces that are more appropriate, either ecb(...) or a
>> library interface, which may be more appropriate in some cases. In any
>> case, the end result should be that ciphers are no longer used outside
>> of crypto/ and drivers/crypto/
>> 
>> This series is presented as an RFC, since I am mostly interested in
>> discussing the above, but I prefer to do so in the context of actual
>> patches rather than an abstract discussion.
>> 
>> Ard Biesheuvel (3):
>>  net/mac80211: switch to skcipher interface for arc4
>>  lib80211/tkip: switch to skcipher interface for arc4
>>  lib80211/wep: switch to skcipher interface for arc4
>> 
> 
> The way the crypto API exposes ARC4 is definitely broken.  It treats it as a
> block cipher (with a block size of 1 byte...), when it's actually a stream
> cipher.  Also, it violates the API by modifying the key during each encryption.
> 
> Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
> using, and the users call it on virtual addresses, perhaps we should instead
> remove it from the crypto API and provide a library function arc4_crypt()?  We'd
> lose support for ARC4 in three hardware drivers, but are there real users who
> really are using ARC4 and need those to get acceptable performance?  Note that
> they aren't being used in the cases where the 'cipher' API is currently being
> used, so it would only be the current 'skcipher' users that might matter.
> 
> Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
> seems unlikely…

that is not unlikely, we use ecb(arc4) via AF_ALG in iwd. It is what the WiFi standard defines to be used.

Regards

Marcel

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Ard Biesheuvel]

On Fri, 7 Jun 2019 at 22:24, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Eric,
>
> >> One of the issues that I would like to see addressed in the crypto API
> >> is they way the cipher abstraction is used. In general, a cipher should
> >> never be used directly, and so it would be much better to clean up the
> >> existing uses of ciphers outside of the crypto subsystem itself, so that
> >> we can make the cipher abstraction part of the internal API, only to
> >> be used by templates or crypto drivers that require them as a callback.
> >>
> >> As a first step, this series moves all users of the 'arc4' cipher to
> >> the ecb(arc4) skcipher, which happens to be implemented by the same
> >> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> >> actually evaluates to 1.
> >>
> >> Next step would be to switch the users of the 'des' and 'aes' ciphers
> >> to other interfaces that are more appropriate, either ecb(...) or a
> >> library interface, which may be more appropriate in some cases. In any
> >> case, the end result should be that ciphers are no longer used outside
> >> of crypto/ and drivers/crypto/
> >>
> >> This series is presented as an RFC, since I am mostly interested in
> >> discussing the above, but I prefer to do so in the context of actual
> >> patches rather than an abstract discussion.
> >>
> >> Ard Biesheuvel (3):
> >>  net/mac80211: switch to skcipher interface for arc4
> >>  lib80211/tkip: switch to skcipher interface for arc4
> >>  lib80211/wep: switch to skcipher interface for arc4
> >>
> >
> > The way the crypto API exposes ARC4 is definitely broken.  It treats it as a
> > block cipher (with a block size of 1 byte...), when it's actually a stream
> > cipher.  Also, it violates the API by modifying the key during each encryption.
> >
> > Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
> > using, and the users call it on virtual addresses, perhaps we should instead
> > remove it from the crypto API and provide a library function arc4_crypt()?  We'd
> > lose support for ARC4 in three hardware drivers, but are there real users who
> > really are using ARC4 and need those to get acceptable performance?  Note that
> > they aren't being used in the cases where the 'cipher' API is currently being
> > used, so it would only be the current 'skcipher' users that might matter.
> >
> > Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
> > seems unlikely…
>
> that is not unlikely, we use ecb(arc4) via AF_ALG in iwd. It is what the WiFi standard defines to be used.
>

Ah ok, good to know. That does imply that the driver is not entirely
broken, which is good news I suppose.

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Denis Kenzior]

Hi Ard,

> 
> Ah ok, good to know. That does imply that the driver is not entirely
> broken, which is good news I suppose.
> 

Not entirely, but we did have to resort to using multiple sockets, 
otherwise parallel encrypt/decrypt operations on the socket would result 
in invalid behavior.  Probably due to the issue Eric already pointed out.

No such issue with any other ciphers that we use.

Regards,
-Denis

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Eric Biggers]

On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> Hi Ard,
> 
> > 
> > Ah ok, good to know. That does imply that the driver is not entirely
> > broken, which is good news I suppose.
> > 
> 
> Not entirely, but we did have to resort to using multiple sockets, otherwise
> parallel encrypt/decrypt operations on the socket would result in invalid
> behavior.  Probably due to the issue Eric already pointed out.
> 
> No such issue with any other ciphers that we use.
> 
> Regards,
> -Denis

Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then.  And
we can't fix its name to be just "arc4".  It's odd that someone would choose to
use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.

Yes, "ecb(arc4)" isn't currently thread safe.  ARC4 uses a single key whereas
modern stream ciphers use a key + IV.  To comply with the crypto API it would
have to copy the key to a stack buffer for each encryption/decryption.  But it
doesn't; it just updates the key instead, making it non thread safe.  If users
are actually relying on that, we'll have to settle for adding a mutex instead.

In any case, we can still remove the 'cipher' algorithm version as Ard is
suggesting, as well as possibly convert the in-kernel users to use an
arc4_crypt() library function and remove the hardware driver support.

- Eric

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Denis Kenzior]

Hi Eric,

On 06/07/2019 04:15 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>> Hi Ard,
>>
>>>
>>> Ah ok, good to know. That does imply that the driver is not entirely
>>> broken, which is good news I suppose.
>>>
>>
>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>> parallel encrypt/decrypt operations on the socket would result in invalid
>> behavior.  Probably due to the issue Eric already pointed out.
>>
>> No such issue with any other ciphers that we use.
>>
>> Regards,
>> -Denis
> 
> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then.  And
> we can't fix its name to be just "arc4".  It's odd that someone would choose to
> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
> 
> Yes, "ecb(arc4)" isn't currently thread safe.  ARC4 uses a single key whereas
> modern stream ciphers use a key + IV.  To comply with the crypto API it would
> have to copy the key to a stack buffer for each encryption/decryption.  But it
> doesn't; it just updates the key instead, making it non thread safe.  If users
> are actually relying on that, we'll have to settle for adding a mutex instead.

Well the issue isn't even about being thread safe.  We run a single 
thread in iwd.  The details are a bit fuzzy now due to time elapsed, but 
if I recall correctly, even behavior like:

fd = socket();
bind(fd, ecb(arc4));
setsockopt(fd, ...key...);

sendmsg(fd, OP_ENCRYPT, ...);
sendmsg(fd, OP_DECRYPT, ...);
sendmsg(fd, OP_ENCRYPT, ...);

would produce different (incorrect) encrypted results compared to

sendmsg(fd, OP_ENCRYPT, ...)
sendmsg(fd, OP_ENCRYPT, ...)

Regards,
-Denis

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Eric Biggers]

On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
> Hi Eric,
> 
> On 06/07/2019 04:15 PM, Eric Biggers wrote:
> > On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> > > Hi Ard,
> > > 
> > > > 
> > > > Ah ok, good to know. That does imply that the driver is not entirely
> > > > broken, which is good news I suppose.
> > > > 
> > > 
> > > Not entirely, but we did have to resort to using multiple sockets, otherwise
> > > parallel encrypt/decrypt operations on the socket would result in invalid
> > > behavior.  Probably due to the issue Eric already pointed out.
> > > 
> > > No such issue with any other ciphers that we use.
> > > 
> > > Regards,
> > > -Denis
> > 
> > Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then.  And
> > we can't fix its name to be just "arc4".  It's odd that someone would choose to
> > use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
> > 
> > Yes, "ecb(arc4)" isn't currently thread safe.  ARC4 uses a single key whereas
> > modern stream ciphers use a key + IV.  To comply with the crypto API it would
> > have to copy the key to a stack buffer for each encryption/decryption.  But it
> > doesn't; it just updates the key instead, making it non thread safe.  If users
> > are actually relying on that, we'll have to settle for adding a mutex instead.
> 
> Well the issue isn't even about being thread safe.  We run a single thread
> in iwd.  The details are a bit fuzzy now due to time elapsed, but if I
> recall correctly, even behavior like:
> 
> fd = socket();
> bind(fd, ecb(arc4));
> setsockopt(fd, ...key...);
> 
> sendmsg(fd, OP_ENCRYPT, ...);
> sendmsg(fd, OP_DECRYPT, ...);
> sendmsg(fd, OP_ENCRYPT, ...);
> 
> would produce different (incorrect) encrypted results compared to
> 
> sendmsg(fd, OP_ENCRYPT, ...)
> sendmsg(fd, OP_ENCRYPT, ...)
> 

That's because currently each operation uses the next bytes from the keystream,
and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
There's no difference between ARC4 encryption and decryption; both just XOR the
keystream with the data.  Are you saying you expected each encryption to be a
continuation of the previous encryption, but decryptions to be independent?

- Eric

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Denis Kenzior]

Hi Eric,

On 06/07/2019 04:41 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
>> Hi Eric,
>>
>> On 06/07/2019 04:15 PM, Eric Biggers wrote:
>>> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>>>> Hi Ard,
>>>>
>>>>>
>>>>> Ah ok, good to know. That does imply that the driver is not entirely
>>>>> broken, which is good news I suppose.
>>>>>
>>>>
>>>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>>>> parallel encrypt/decrypt operations on the socket would result in invalid
>>>> behavior.  Probably due to the issue Eric already pointed out.
>>>>
>>>> No such issue with any other ciphers that we use.
>>>>
>>>> Regards,
>>>> -Denis
>>>
>>> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then.  And
>>> we can't fix its name to be just "arc4".  It's odd that someone would choose to
>>> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
>>>
>>> Yes, "ecb(arc4)" isn't currently thread safe.  ARC4 uses a single key whereas
>>> modern stream ciphers use a key + IV.  To comply with the crypto API it would
>>> have to copy the key to a stack buffer for each encryption/decryption.  But it
>>> doesn't; it just updates the key instead, making it non thread safe.  If users
>>> are actually relying on that, we'll have to settle for adding a mutex instead.
>>
>> Well the issue isn't even about being thread safe.  We run a single thread
>> in iwd.  The details are a bit fuzzy now due to time elapsed, but if I
>> recall correctly, even behavior like:
>>
>> fd = socket();
>> bind(fd, ecb(arc4));
>> setsockopt(fd, ...key...);
>>
>> sendmsg(fd, OP_ENCRYPT, ...);
>> sendmsg(fd, OP_DECRYPT, ...);
>> sendmsg(fd, OP_ENCRYPT, ...);
>>
>> would produce different (incorrect) encrypted results compared to
>>
>> sendmsg(fd, OP_ENCRYPT, ...)
>> sendmsg(fd, OP_ENCRYPT, ...)
>>
> 
> That's because currently each operation uses the next bytes from the keystream,
> and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
> There's no difference between ARC4 encryption and decryption; both just XOR the
> keystream with the data.  Are you saying you expected each encryption to be a
> continuation of the previous encryption, but decryptions to be independent?
> 

 From a userspace / api perspective, yes I would have expected the 
encrypt and decrypt to work independently.  No biggie now, but I 
remember being surprised when this bit me as no other cipher had this 
behavior.  E.g. interleaving of operations seemed to only affect arc4 
results.

Are the exact semantics spelled out somewhere?

Regards,
-Denis

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Eric Biggers]

On Fri, Jun 07, 2019 at 04:54:04PM -0500, Denis Kenzior wrote:
> Hi Eric,
> 
> On 06/07/2019 04:41 PM, Eric Biggers wrote:
> > On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
> > > Hi Eric,
> > > 
> > > On 06/07/2019 04:15 PM, Eric Biggers wrote:
> > > > On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> > > > > Hi Ard,
> > > > > 
> > > > > > 
> > > > > > Ah ok, good to know. That does imply that the driver is not entirely
> > > > > > broken, which is good news I suppose.
> > > > > > 
> > > > > 
> > > > > Not entirely, but we did have to resort to using multiple sockets, otherwise
> > > > > parallel encrypt/decrypt operations on the socket would result in invalid
> > > > > behavior.  Probably due to the issue Eric already pointed out.
> > > > > 
> > > > > No such issue with any other ciphers that we use.
> > > > > 
> > > > > Regards,
> > > > > -Denis
> > > > 
> > > > Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then.  And
> > > > we can't fix its name to be just "arc4".  It's odd that someone would choose to
> > > > use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
> > > > 
> > > > Yes, "ecb(arc4)" isn't currently thread safe.  ARC4 uses a single key whereas
> > > > modern stream ciphers use a key + IV.  To comply with the crypto API it would
> > > > have to copy the key to a stack buffer for each encryption/decryption.  But it
> > > > doesn't; it just updates the key instead, making it non thread safe.  If users
> > > > are actually relying on that, we'll have to settle for adding a mutex instead.
> > > 
> > > Well the issue isn't even about being thread safe.  We run a single thread
> > > in iwd.  The details are a bit fuzzy now due to time elapsed, but if I
> > > recall correctly, even behavior like:
> > > 
> > > fd = socket();
> > > bind(fd, ecb(arc4));
> > > setsockopt(fd, ...key...);
> > > 
> > > sendmsg(fd, OP_ENCRYPT, ...);
> > > sendmsg(fd, OP_DECRYPT, ...);
> > > sendmsg(fd, OP_ENCRYPT, ...);
> > > 
> > > would produce different (incorrect) encrypted results compared to
> > > 
> > > sendmsg(fd, OP_ENCRYPT, ...)
> > > sendmsg(fd, OP_ENCRYPT, ...)
> > > 
> > 
> > That's because currently each operation uses the next bytes from the keystream,
> > and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
> > There's no difference between ARC4 encryption and decryption; both just XOR the
> > keystream with the data.  Are you saying you expected each encryption to be a
> > continuation of the previous encryption, but decryptions to be independent?
> > 
> 
> From a userspace / api perspective, yes I would have expected the encrypt
> and decrypt to work independently.  No biggie now, but I remember being
> surprised when this bit me as no other cipher had this behavior.  E.g.
> interleaving of operations seemed to only affect arc4 results.
> 
> Are the exact semantics spelled out somewhere?
> 

For all other skcipher algorithms, every operation is independent and depends
only on the key which was set previously on the algorithm socket, plus the IV
provided for the operation.  There is no way to perform a single encryption or
decryption incrementally in multiple parts, unless the algorithm supports it
naturally by updating the IV (e.g. CBC mode).

As I am attempting to explain, ecb(arc4) does not implement this API correctly
because it updates the *key* after each operation, not the IV.  I doubt this is
documented anywhere, but this can only be changed if people aren't relying on it
already.

- Eric

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Denis Kenzior]

Hi Eric,

On 06/07/2019 05:40 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 04:54:04PM -0500, Denis Kenzior wrote:
>> Hi Eric,
>>
>> On 06/07/2019 04:41 PM, Eric Biggers wrote:
>>> On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
>>>> Hi Eric,
>>>>
>>>> On 06/07/2019 04:15 PM, Eric Biggers wrote:
>>>>> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>>>
>>>>>>> Ah ok, good to know. That does imply that the driver is not entirely
>>>>>>> broken, which is good news I suppose.
>>>>>>>
>>>>>>
>>>>>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>>>>>> parallel encrypt/decrypt operations on the socket would result in invalid
>>>>>> behavior.  Probably due to the issue Eric already pointed out.
>>>>>>
>>>>>> No such issue with any other ciphers that we use.
>>>>>>
>>>>>> Regards,
>>>>>> -Denis
>>>>>
>>>>> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then.  And
>>>>> we can't fix its name to be just "arc4".  It's odd that someone would choose to
>>>>> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
>>>>>
>>>>> Yes, "ecb(arc4)" isn't currently thread safe.  ARC4 uses a single key whereas
>>>>> modern stream ciphers use a key + IV.  To comply with the crypto API it would
>>>>> have to copy the key to a stack buffer for each encryption/decryption.  But it
>>>>> doesn't; it just updates the key instead, making it non thread safe.  If users
>>>>> are actually relying on that, we'll have to settle for adding a mutex instead.
>>>>
>>>> Well the issue isn't even about being thread safe.  We run a single thread
>>>> in iwd.  The details are a bit fuzzy now due to time elapsed, but if I
>>>> recall correctly, even behavior like:
>>>>
>>>> fd = socket();
>>>> bind(fd, ecb(arc4));
>>>> setsockopt(fd, ...key...);
>>>>
>>>> sendmsg(fd, OP_ENCRYPT, ...);
>>>> sendmsg(fd, OP_DECRYPT, ...);
>>>> sendmsg(fd, OP_ENCRYPT, ...);
>>>>
>>>> would produce different (incorrect) encrypted results compared to
>>>>
>>>> sendmsg(fd, OP_ENCRYPT, ...)
>>>> sendmsg(fd, OP_ENCRYPT, ...)
>>>>
>>>
>>> That's because currently each operation uses the next bytes from the keystream,
>>> and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
>>> There's no difference between ARC4 encryption and decryption; both just XOR the
>>> keystream with the data.  Are you saying you expected each encryption to be a
>>> continuation of the previous encryption, but decryptions to be independent?
>>>
>>
>>  From a userspace / api perspective, yes I would have expected the encrypt
>> and decrypt to work independently.  No biggie now, but I remember being
>> surprised when this bit me as no other cipher had this behavior.  E.g.
>> interleaving of operations seemed to only affect arc4 results.
>>
>> Are the exact semantics spelled out somewhere?
>>
> 
> For all other skcipher algorithms, every operation is independent and depends
> only on the key which was set previously on the algorithm socket, plus the IV
> provided for the operation.  There is no way to perform a single encryption or
> decryption incrementally in multiple parts, unless the algorithm supports it
> naturally by updating the IV (e.g. CBC mode).

Right, that is what I thought.

> 
> As I am attempting to explain, ecb(arc4) does not implement this API correctly
> because it updates the *key* after each operation, not the IV.  I doubt this is
> documented anywhere, but this can only be changed if people aren't relying on it
> already.

It sounds to me like it was broken and should be fixed.  So our vote / 
preference is to have ARC4 fixed to follow the proper semantics.  We can 
deal with the kernel behavioral change on our end easily enough; the 
required workarounds are the worse evil.

Regards,
-Denis

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Sandy Harris]

First off, it is not clear we should implement WEP at all since it is
fatally flawed. This has been known for about a decade, there have
been at least two better algorithms added to the standards, & the only
reason anyone would need WEP today would be to connect to an old
router in an obviously insecure way.
https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html
https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html

Twenty years ago the FreeS/WAN project implemented IPsec for Linux &
deliberately did not include things like single DES which were known
to be insecure:
https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped
I think a similar policy was would be a fine idea for the kernel today
& WEP is hopelessly insecure.

> > As I am attempting to explain, ecb(arc4) does not implement this API correctly
> > because it updates the *key* after each operation, not the IV.  I doubt this is
> > documented anywhere, but this can only be changed if people aren't relying on it
> > already.

It is more the case that the API does not apply to arc4, or more
generally to stream ciphers, than that "ecb(arc4) does not implement
this API correctly".

ECB (electronic code book) is a mode of operation for block ciphers
https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
Stream ciphers do not have those modes.

For that matter, not all block cipher modes use an IV. The very common
CBC mode -- the only mode used in IPsec, for example -- does, but
others including ECB do not. I do not know of any mode that ever
updates the IV. CBC uses the IV with the first block & on all other
blocks uses the ciphertext from the previous block the same way; one
might call that updating the IV I suppose, but I do not see why one
would want to.

> It sounds to me like it was broken and should be fixed.  So our vote /
> preference is to have ARC4 fixed to follow the proper semantics.

As I see it, those are clearly not "he proper semantics" for a stream
cipher & the question of forcing it into them should not even arise.

One alternative would be to drop arc4. That would make sense if WEP is
the only usage & we elect to drop WEP. One could also argue the arc4
itself is insecure & should go, but I'm not sure that is accurate.
Certainly there have been some published attacks & other stream
ciphers are now generally preferrred, but I have not followed things
closely enough to know if RC$ should be considered fatally flawed.

A better choice might be to change the interface, defining a new
interface for stream ciphers and/or generalising the interface so it
works for either stream ciphers or block ciphers.

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Ard Biesheuvel]

On Sat, 8 Jun 2019 at 15:03, Sandy Harris <sandyinchina@gmail.com> wrote:
>
> First off, it is not clear we should implement WEP at all since it is
> fatally flawed. This has been known for about a decade, there have
> been at least two better algorithms added to the standards, & the only
> reason anyone would need WEP today would be to connect to an old
> router in an obviously insecure way.
> https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html
> https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html
>
> Twenty years ago the FreeS/WAN project implemented IPsec for Linux &
> deliberately did not include things like single DES which were known
> to be insecure:
> https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped
> I think a similar policy was would be a fine idea for the kernel today
> & WEP is hopelessly insecure.
>

It is actually pretty clear that we should implement WEP, simply
because we already do. We all know how broken it is, but that does not
mean we should be the ones policing its use. People may have good
reasons to stick with WEP in their particular use case, or maybe they
have bad reasons, but the bottom line is that it does not really
matter: if it works today, we can't just remove it.

What we can do is make the existing code less of an eyesore than it
already is, and in the context of what I want to achieve for the
crypto API, this involves moving it from the cipher API to something
else.

> > > As I am attempting to explain, ecb(arc4) does not implement this API correctly
> > > because it updates the *key* after each operation, not the IV.  I doubt this is
> > > documented anywhere, but this can only be changed if people aren't relying on it
> > > already.
>
> It is more the case that the API does not apply to arc4, or more
> generally to stream ciphers, than that "ecb(arc4) does not implement
> this API correctly".
>
> ECB (electronic code book) is a mode of operation for block ciphers
> https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
> Stream ciphers do not have those modes.
>

This is exactly the point Eric was making. Our skcipher abstraction
deals with stream ciphers fine, but the way the arc4 code is exposed
as ecb(arc4) and updates the key in the process makes absolutely no
sense.

> For that matter, not all block cipher modes use an IV. The very common
> CBC mode -- the only mode used in IPsec, for example -- does, but
> others including ECB do not. I do not know of any mode that ever
> updates the IV. CBC uses the IV with the first block & on all other
> blocks uses the ciphertext from the previous block the same way; one
> might call that updating the IV I suppose, but I do not see why one
> would want to.
>

If you want to split up a CBC transformation into several invocations
of the underlying API, then the last ciphertext block of the first
call serves as the IV for the next call. Arguing that we should not be
calling this an IV serves little purpose, since the code already
treats it exactly the same. In fact, our CTS template relies on this
feature as well, so a CBC implementation that does not return the last
ciphertext block in the IV buffer is broken wrt our API requirements.

> > It sounds to me like it was broken and should be fixed.  So our vote /
> > preference is to have ARC4 fixed to follow the proper semantics.
>
> As I see it, those are clearly not "he proper semantics" for a stream
> cipher & the question of forcing it into them should not even arise.
>
> One alternative would be to drop arc4. That would make sense if WEP is
> the only usage & we elect to drop WEP. One could also argue the arc4
> itself is insecure & should go, but I'm not sure that is accurate.
> Certainly there have been some published attacks & other stream
> ciphers are now generally preferrred, but I have not followed things
> closely enough to know if RC$ should be considered fatally flawed.
>
> A better choice might be to change the interface, defining a new
> interface for stream ciphers and/or generalising the interface so it
> works for either stream ciphers or block ciphers.

Dropping WEP is out of the question, and apparently, there are
userspace dependencies on the ecb(arc4) cipher as well, so
unfortunately, we have already painted ourselves into a corner here.

skcipher works fine for block ciphers wrapped in CTR mode, and for
chacha/salsa as well, so I don't think there is a problem with the API
for other stream ciphers we care about. Calling the rc4 skcipher
'ecb(arc4)' was obviously a mistake, but it seems we're stuck with
that as well :-(

Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface

[Ard Biesheuvel]

On Sat, 8 Jun 2019 at 16:37, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Sat, 8 Jun 2019 at 15:03, Sandy Harris <sandyinchina@gmail.com> wrote:
> >
> > First off, it is not clear we should implement WEP at all since it is
> > fatally flawed. This has been known for about a decade, there have
> > been at least two better algorithms added to the standards, & the only
> > reason anyone would need WEP today would be to connect to an old
> > router in an obviously insecure way.
> > https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html
> > https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html
> >
> > Twenty years ago the FreeS/WAN project implemented IPsec for Linux &
> > deliberately did not include things like single DES which were known
> > to be insecure:
> > https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped
> > I think a similar policy was would be a fine idea for the kernel today
> > & WEP is hopelessly insecure.
> >
>
> It is actually pretty clear that we should implement WEP, simply
> because we already do. We all know how broken it is, but that does not
> mean we should be the ones policing its use. People may have good
> reasons to stick with WEP in their particular use case, or maybe they
> have bad reasons, but the bottom line is that it does not really
> matter: if it works today, we can't just remove it.
>
> What we can do is make the existing code less of an eyesore than it
> already is, and in the context of what I want to achieve for the
> crypto API, this involves moving it from the cipher API to something
> else.
>
> > > > As I am attempting to explain, ecb(arc4) does not implement this API correctly
> > > > because it updates the *key* after each operation, not the IV.  I doubt this is
> > > > documented anywhere, but this can only be changed if people aren't relying on it
> > > > already.
> >
> > It is more the case that the API does not apply to arc4, or more
> > generally to stream ciphers, than that "ecb(arc4) does not implement
> > this API correctly".
> >
> > ECB (electronic code book) is a mode of operation for block ciphers
> > https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
> > Stream ciphers do not have those modes.
> >
>
> This is exactly the point Eric was making. Our skcipher abstraction
> deals with stream ciphers fine, but the way the arc4 code is exposed
> as ecb(arc4) and updates the key in the process makes absolutely no
> sense.
>
> > For that matter, not all block cipher modes use an IV. The very common
> > CBC mode -- the only mode used in IPsec, for example -- does, but
> > others including ECB do not. I do not know of any mode that ever
> > updates the IV. CBC uses the IV with the first block & on all other
> > blocks uses the ciphertext from the previous block the same way; one
> > might call that updating the IV I suppose, but I do not see why one
> > would want to.
> >
>
> If you want to split up a CBC transformation into several invocations
> of the underlying API, then the last ciphertext block of the first
> call serves as the IV for the next call. Arguing that we should not be
> calling this an IV serves little purpose, since the code already
> treats it exactly the same. In fact, our CTS template relies on this
> feature as well, so a CBC implementation that does not return the last
> ciphertext block in the IV buffer is broken wrt our API requirements.
>
> > > It sounds to me like it was broken and should be fixed.  So our vote /
> > > preference is to have ARC4 fixed to follow the proper semantics.
> >
> > As I see it, those are clearly not "he proper semantics" for a stream
> > cipher & the question of forcing it into them should not even arise.
> >
> > One alternative would be to drop arc4. That would make sense if WEP is
> > the only usage & we elect to drop WEP. One could also argue the arc4
> > itself is insecure & should go, but I'm not sure that is accurate.
> > Certainly there have been some published attacks & other stream
> > ciphers are now generally preferrred, but I have not followed things
> > closely enough to know if RC$ should be considered fatally flawed.
> >
> > A better choice might be to change the interface, defining a new
> > interface for stream ciphers and/or generalising the interface so it
> > works for either stream ciphers or block ciphers.
>
> Dropping WEP is out of the question, and apparently, there are
> userspace dependencies on the ecb(arc4) cipher as well, so
> unfortunately, we have already painted ourselves into a corner here.
>
> skcipher works fine for block ciphers wrapped in CTR mode, and for
> chacha/salsa as well, so I don't think there is a problem with the API
> for other stream ciphers we care about. Calling the rc4 skcipher
> 'ecb(arc4)' was obviously a mistake, but it seems we're stuck with
> that as well :-(

As it turns out, we have other users of ecb(arc4) in the MPPE code,
the kerberos code and some realtek code in the staging tree. More
interestingly, the code this series changes was recently converted
from skcipher to cipher, while I am doing the opposite.

Given Eric's analysis that few of these users actually take advantage
of the crypto API (i.e., they all use the sync variety and hardcode
the algo name), we can simplify them to use library calls instead. The
only remaining skcipher user would be the Kerberos code, which
dynamically instantiates skciphers with a parameterized algo name.