Message-ID: <Z2KZC71JZ0QnrhfU@gondor.apana.org.au>
Date: Wed, 18 Dec 2024 17:42:35 +0800
From: Herbert Xu <herbert@...dor.apana.org.au>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Steffen Klassert <steffen.klassert@...unet.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: [PATCH net] xfrm: Rewrite key length conversion to avoid overflows

On Tue, Dec 17, 2024 at 03:32:31PM +0300, Dan Carpenter wrote:
>
> That seems like basic algebra but we have a long history of getting
> integer overflow checks wrong so these days I like to just use
> INT_MAX where ever I can.  I wanted to use USHRT_MAX. We aren't allowed
> to use more than USHRT_MAX bytes, but maybe we're allowed USHRT_MAX
> bits, so I didn't do that.

There is no reason for this to overflow if we rewrite it do do
the division carefully.  Something like this:

Steffen, this raises a new question: Can normal users create socket
policies of arbtirarily long key lengths? If so we probably should
look into limiting the key length to a sane value.  Of course, given
namespaces we probably should do that in any case.

---8<---
Rewrite the IPsec conversion from bit-length to byte-length so that
large values do not overflow.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c
index c7338ac6a5bb..eb7bb196d75d 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ipsec/chcr_ipsec.c
@@ -165,7 +165,7 @@ static int ch_ipsec_setauthsize(struct xfrm_state *x,
 static int ch_ipsec_setkey(struct xfrm_state *x,
 			   struct ipsec_sa_entry *sa_entry)
 {
-	int keylen = (x->aead->alg_key_len + 7) / 8;
+	int keylen = xfrm_kblen2klen(x->aead->alg_key_len);
 	unsigned char *key = x->aead->alg_key;
 	int ck_size, key_ctx_size = 0;
 	unsigned char ghash_h[AEAD_H_SIZE];
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
index ca92e518be76..24c96a4497b2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -323,7 +323,7 @@ void mlx5e_ipsec_build_accel_xfrm_attrs(struct mlx5e_ipsec_sa_entry *sa_entry,
 	memset(attrs, 0, sizeof(*attrs));
 
 	/* key */
-	crypto_data_len = (x->aead->alg_key_len + 7) / 8;
+	crypto_data_len = xfrm_kblen2klen(x->aead->alg_key_len);
 	key_len = crypto_data_len - 4; /* 4 bytes salt at end */
 
 	memcpy(aes_gcm->aes_key, x->aead->alg_key, key_len);
diff --git a/drivers/net/ethernet/netronome/nfp/crypto/ipsec.c b/drivers/net/ethernet/netronome/nfp/crypto/ipsec.c
index 515069d5637b..2660236eb07f 100644
--- a/drivers/net/ethernet/netronome/nfp/crypto/ipsec.c
+++ b/drivers/net/ethernet/netronome/nfp/crypto/ipsec.c
@@ -365,7 +365,7 @@ static int nfp_net_xfrm_add_state(struct xfrm_state *x,
 	}
 
 	if (x->aalg) {
-		key_len = DIV_ROUND_UP(x->aalg->alg_key_len, BITS_PER_BYTE);
+		key_len = xfrm_kblen2klen(x->aalg->alg_key_len);
 		if (key_len > sizeof(cfg->auth_key)) {
 			NL_SET_ERR_MSG_MOD(extack, "Insufficient space for offloaded auth key");
 			return -EINVAL;
@@ -458,7 +458,7 @@ static int nfp_net_xfrm_add_state(struct xfrm_state *x,
 		int key_offset = 0;
 		int salt_len = 4;
 
-		key_len = DIV_ROUND_UP(x->aead->alg_key_len, BITS_PER_BYTE);
+		key_len = xfrm_kblen2klen(x->aead->alg_key_len);
 		key_len -= salt_len;
 
 		if (key_len > sizeof(cfg->ciph_key)) {
@@ -485,7 +485,7 @@ static int nfp_net_xfrm_add_state(struct xfrm_state *x,
 	}
 
 	if (x->ealg) {
-		key_len = DIV_ROUND_UP(x->ealg->alg_key_len, BITS_PER_BYTE);
+		key_len = xfrm_kblen2klen(x->ealg->alg_key_len);
 
 		if (key_len > sizeof(cfg->ciph_key)) {
 			NL_SET_ERR_MSG_MOD(extack, "ealg: Insufficient space for offloaded key");
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 32c09e85a64c..1ce897a9476b 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1912,19 +1912,24 @@ static inline int xfrm_acquire_is_on(struct net *net)
 }
 #endif
 
+static inline unsigned int xfrm_kblen2klen(unsigned int klen_in_bits)
+{
+	return klen_in_bits / 8 + !!(klen_in_bits & 7);
+}
+
 static inline unsigned int aead_len(struct xfrm_algo_aead *alg)
 {
-	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);
+	return sizeof(*alg) + xfrm_kblen2klen(alg->alg_key_len);
 }
 
 static inline unsigned int xfrm_alg_len(const struct xfrm_algo *alg)
 {
-	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);
+	return sizeof(*alg) + xfrm_kblen2klen(alg->alg_key_len);
 }
 
 static inline unsigned int xfrm_alg_auth_len(const struct xfrm_algo_auth *alg)
 {
-	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);
+	return sizeof(*alg) + xfrm_kblen2klen(alg->alg_key_len);
 }
 
 static inline unsigned int xfrm_replay_state_esn_len(struct xfrm_replay_state_esn *replay_esn)
diff --git a/net/ipv4/ah4.c b/net/ipv4/ah4.c
index 64aec3dff8ec..efea16f28b8d 100644
--- a/net/ipv4/ah4.c
+++ b/net/ipv4/ah4.c
@@ -496,7 +496,7 @@ static int ah_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)
 
 	ahp->ahash = ahash;
 	if (crypto_ahash_setkey(ahash, x->aalg->alg_key,
-				(x->aalg->alg_key_len + 7) / 8)) {
+				xfrm_kblen2klen(x->aalg->alg_key_len))) {
 		NL_SET_ERR_MSG(extack, "Kernel was unable to initialize cryptographic operations");
 		goto error;
 	}
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index f3281312eb5e..6dcbaf75c8b6 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -1028,7 +1028,7 @@ static int esp_init_aead(struct xfrm_state *x, struct netlink_ext_ack *extack)
 	x->data = aead;
 
 	err = crypto_aead_setkey(aead, x->aead->alg_key,
-				 (x->aead->alg_key_len + 7) / 8);
+				 xfrm_kblen2klen(x->aead->alg_key_len));
 	if (err)
 		goto error;
 
@@ -1088,8 +1088,9 @@ static int esp_init_authenc(struct xfrm_state *x,
 
 	x->data = aead;
 
-	keylen = (x->aalg ? (x->aalg->alg_key_len + 7) / 8 : 0) +
-		 (x->ealg->alg_key_len + 7) / 8 + RTA_SPACE(sizeof(*param));
+	keylen = x->aalg ? xfrm_kblen2klen(x->aalg->alg_key_len) : 0;
+	keylen += xfrm_kblen2klen(x->ealg->alg_key_len);
+	keylen += RTA_SPACE(sizeof(*param));
 	err = -ENOMEM;
 	key = kmalloc(keylen, GFP_KERNEL);
 	if (!key)
@@ -1105,8 +1106,8 @@ static int esp_init_authenc(struct xfrm_state *x,
 	if (x->aalg) {
 		struct xfrm_algo_desc *aalg_desc;
 
-		memcpy(p, x->aalg->alg_key, (x->aalg->alg_key_len + 7) / 8);
-		p += (x->aalg->alg_key_len + 7) / 8;
+		memcpy(p, x->aalg->alg_key, xfrm_kblen2klen(x->aalg->alg_key_len));
+		p += xfrm_kblen2klen(x->aalg->alg_key_len);
 
 		aalg_desc = xfrm_aalg_get_byname(x->aalg->alg_name, 0);
 		BUG_ON(!aalg_desc);
@@ -1126,8 +1127,8 @@ static int esp_init_authenc(struct xfrm_state *x,
 		}
 	}
 
-	param->enckeylen = cpu_to_be32((x->ealg->alg_key_len + 7) / 8);
-	memcpy(p, x->ealg->alg_key, (x->ealg->alg_key_len + 7) / 8);
+	param->enckeylen = cpu_to_be32(xfrm_kblen2klen(x->ealg->alg_key_len));
+	memcpy(p, x->ealg->alg_key, xfrm_kblen2klen(x->ealg->alg_key_len));
 
 	err = crypto_aead_setkey(aead, key, keylen);
 
diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index eb474f0987ae..edf46ba35823 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -691,7 +691,7 @@ static int ah6_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)
 
 	ahp->ahash = ahash;
 	if (crypto_ahash_setkey(ahash, x->aalg->alg_key,
-			       (x->aalg->alg_key_len + 7) / 8)) {
+				xfrm_kblen2klen(x->aalg->alg_key_len))) {
 		NL_SET_ERR_MSG(extack, "Kernel was unable to initialize cryptographic operations");
 		goto error;
 	}
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index b2400c226a32..1cd8c4f71d33 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -1065,7 +1065,7 @@ static int esp_init_aead(struct xfrm_state *x, struct netlink_ext_ack *extack)
 	x->data = aead;
 
 	err = crypto_aead_setkey(aead, x->aead->alg_key,
-				 (x->aead->alg_key_len + 7) / 8);
+				 xfrm_kblen2klen(x->aead->alg_key_len));
 	if (err)
 		goto error;
 
@@ -1125,8 +1125,9 @@ static int esp_init_authenc(struct xfrm_state *x,
 
 	x->data = aead;
 
-	keylen = (x->aalg ? (x->aalg->alg_key_len + 7) / 8 : 0) +
-		 (x->ealg->alg_key_len + 7) / 8 + RTA_SPACE(sizeof(*param));
+	keylen = (x->aalg ? xfrm_kblen2klen(x->aalg->alg_key_len) : 0);
+	keylen += xfrm_kblen2klen(x->ealg->alg_key_len);
+	keylen += RTA_SPACE(sizeof(*param));
 	err = -ENOMEM;
 	key = kmalloc(keylen, GFP_KERNEL);
 	if (!key)
@@ -1142,8 +1143,8 @@ static int esp_init_authenc(struct xfrm_state *x,
 	if (x->aalg) {
 		struct xfrm_algo_desc *aalg_desc;
 
-		memcpy(p, x->aalg->alg_key, (x->aalg->alg_key_len + 7) / 8);
-		p += (x->aalg->alg_key_len + 7) / 8;
+		memcpy(p, x->aalg->alg_key, xfrm_kblen2klen(x->aalg->alg_key_len));
+		p += xfrm_kblen2klen(x->aalg->alg_key_len);
 
 		aalg_desc = xfrm_aalg_get_byname(x->aalg->alg_name, 0);
 		BUG_ON(!aalg_desc);
@@ -1163,8 +1164,8 @@ static int esp_init_authenc(struct xfrm_state *x,
 		}
 	}
 
-	param->enckeylen = cpu_to_be32((x->ealg->alg_key_len + 7) / 8);
-	memcpy(p, x->ealg->alg_key, (x->ealg->alg_key_len + 7) / 8);
+	param->enckeylen = cpu_to_be32(xfrm_kblen2klen(x->ealg->alg_key_len));
+	memcpy(p, x->ealg->alg_key, xfrm_kblen2klen(x->ealg->alg_key_len));
 
 	err = crypto_aead_setkey(aead, key, keylen);
 
diff --git a/net/key/af_key.c b/net/key/af_key.c
index c56bb4f451e6..2d0b11780263 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -803,13 +803,11 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 
 	if (add_keys) {
 		if (x->aalg && x->aalg->alg_key_len) {
-			auth_key_size =
-				PFKEY_ALIGN8((x->aalg->alg_key_len + 7) / 8);
+			auth_key_size = PFKEY_ALIGN8(xfrm_kblen2klen(x->aalg->alg_key_len));
 			size += sizeof(struct sadb_key) + auth_key_size;
 		}
 		if (x->ealg && x->ealg->alg_key_len) {
-			encrypt_key_size =
-				PFKEY_ALIGN8((x->ealg->alg_key_len+7) / 8);
+			encrypt_key_size = PFKEY_ALIGN8(xfrm_kblen2klen(x->ealg->alg_key_len));
 			size += sizeof(struct sadb_key) + encrypt_key_size;
 		}
 	}
@@ -967,7 +965,8 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 		key->sadb_key_exttype = SADB_EXT_KEY_AUTH;
 		key->sadb_key_bits = x->aalg->alg_key_len;
 		key->sadb_key_reserved = 0;
-		memcpy(key + 1, x->aalg->alg_key, (x->aalg->alg_key_len+7)/8);
+		memcpy(key + 1, x->aalg->alg_key,
+		       xfrm_kblen2klen(x->aalg->alg_key_len));
 	}
 	/* encrypt key */
 	if (add_keys && encrypt_key_size) {
@@ -978,7 +977,7 @@ static struct sk_buff *__pfkey_xfrm_state2msg(const struct xfrm_state *x,
 		key->sadb_key_bits = x->ealg->alg_key_len;
 		key->sadb_key_reserved = 0;
 		memcpy(key + 1, x->ealg->alg_key,
-		       (x->ealg->alg_key_len+7)/8);
+		       xfrm_kblen2klen(x->ealg->alg_key_len));
 	}
 
 	/* sa */
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b2876e09328b..8d98bf7c7ad1 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -532,6 +532,7 @@ static int attach_auth(struct xfrm_algo_auth **algpp, u8 *props,
 	struct xfrm_algo *ualg;
 	struct xfrm_algo_auth *p;
 	struct xfrm_algo_desc *algo;
+	unsigned int klen;
 
 	if (!rta)
 		return 0;
@@ -545,14 +546,15 @@ static int attach_auth(struct xfrm_algo_auth **algpp, u8 *props,
 	}
 	*props = algo->desc.sadb_alg_id;
 
-	p = kmalloc(sizeof(*p) + (ualg->alg_key_len + 7) / 8, GFP_KERNEL);
+	klen = xfrm_kblen2klen(ualg->alg_key_len);
+	p = kmalloc(sizeof(*p) + klen, GFP_KERNEL);
 	if (!p)
 		return -ENOMEM;
 
 	strcpy(p->alg_name, algo->name);
 	p->alg_key_len = ualg->alg_key_len;
 	p->alg_trunc_len = algo->uinfo.auth.icv_truncbits;
-	memcpy(p->alg_key, ualg->alg_key, (ualg->alg_key_len + 7) / 8);
+	memcpy(p->alg_key, ualg->alg_key, klen);
 
 	*algpp = p;
 	return 0;
@@ -1089,23 +1091,22 @@ static bool xfrm_redact(void)
 
 static int copy_to_user_auth(struct xfrm_algo_auth *auth, struct sk_buff *skb)
 {
+	unsigned int klen = xfrm_kblen2klen(auth->alg_key_len);
 	struct xfrm_algo *algo;
 	struct xfrm_algo_auth *ap;
 	struct nlattr *nla;
 	bool redact_secret = xfrm_redact();
 
-	nla = nla_reserve(skb, XFRMA_ALG_AUTH,
-			  sizeof(*algo) + (auth->alg_key_len + 7) / 8);
+	nla = nla_reserve(skb, XFRMA_ALG_AUTH, sizeof(*algo) + klen);
 	if (!nla)
 		return -EMSGSIZE;
 	algo = nla_data(nla);
 	strscpy_pad(algo->alg_name, auth->alg_name, sizeof(algo->alg_name));
 
-	if (redact_secret && auth->alg_key_len)
-		memset(algo->alg_key, 0, (auth->alg_key_len + 7) / 8);
+	if (redact_secret)
+		memset(algo->alg_key, 0, klen);
 	else
-		memcpy(algo->alg_key, auth->alg_key,
-		       (auth->alg_key_len + 7) / 8);
+		memcpy(algo->alg_key, auth->alg_key, klen);
 	algo->alg_key_len = auth->alg_key_len;
 
 	nla = nla_reserve(skb, XFRMA_ALG_AUTH_TRUNC, xfrm_alg_auth_len(auth));
@@ -1115,16 +1116,16 @@ static int copy_to_user_auth(struct xfrm_algo_auth *auth, struct sk_buff *skb)
 	strscpy_pad(ap->alg_name, auth->alg_name, sizeof(ap->alg_name));
 	ap->alg_key_len = auth->alg_key_len;
 	ap->alg_trunc_len = auth->alg_trunc_len;
-	if (redact_secret && auth->alg_key_len)
-		memset(ap->alg_key, 0, (auth->alg_key_len + 7) / 8);
+	if (redact_secret)
+		memset(ap->alg_key, 0, klen);
 	else
-		memcpy(ap->alg_key, auth->alg_key,
-		       (auth->alg_key_len + 7) / 8);
+		memcpy(ap->alg_key, auth->alg_key, klen);
 	return 0;
 }
 
 static int copy_to_user_aead(struct xfrm_algo_aead *aead, struct sk_buff *skb)
 {
+	unsigned int klen = xfrm_kblen2klen(aead->alg_key_len);
 	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_AEAD, aead_len(aead));
 	struct xfrm_algo_aead *ap;
 	bool redact_secret = xfrm_redact();
@@ -1137,16 +1138,16 @@ static int copy_to_user_aead(struct xfrm_algo_aead *aead, struct sk_buff *skb)
 	ap->alg_key_len = aead->alg_key_len;
 	ap->alg_icv_len = aead->alg_icv_len;
 
-	if (redact_secret && aead->alg_key_len)
-		memset(ap->alg_key, 0, (aead->alg_key_len + 7) / 8);
+	if (redact_secret)
+		memset(ap->alg_key, 0, klen);
 	else
-		memcpy(ap->alg_key, aead->alg_key,
-		       (aead->alg_key_len + 7) / 8);
+		memcpy(ap->alg_key, aead->alg_key, klen);
 	return 0;
 }
 
 static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
 {
+	unsigned int klen = xfrm_kblen2klen(ealg->alg_key_len);
 	struct xfrm_algo *ap;
 	bool redact_secret = xfrm_redact();
 	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_CRYPT,
@@ -1158,11 +1159,10 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
 	strscpy_pad(ap->alg_name, ealg->alg_name, sizeof(ap->alg_name));
 	ap->alg_key_len = ealg->alg_key_len;
 
-	if (redact_secret && ealg->alg_key_len)
-		memset(ap->alg_key, 0, (ealg->alg_key_len + 7) / 8);
+	if (redact_secret)
+		memset(ap->alg_key, 0, klen);
 	else
-		memcpy(ap->alg_key, ealg->alg_key,
-		       (ealg->alg_key_len + 7) / 8);
+		memcpy(ap->alg_key, ealg->alg_key, klen);
 
 	return 0;
 }
@@ -3509,7 +3509,7 @@ static inline unsigned int xfrm_sa_len(struct xfrm_state *x)
 		l += nla_total_size(aead_len(x->aead));
 	if (x->aalg) {
 		l += nla_total_size(sizeof(struct xfrm_algo) +
-				    (x->aalg->alg_key_len + 7) / 8);
+				    xfrm_kblen2klen(x->aalg->alg_key_len));
 		l += nla_total_size(xfrm_alg_auth_len(x->aalg));
 	}
 	if (x->ealg)
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Powered by blists - more mailing lists