[PATCH net] xfrm: prevent some integer overflows in verify_ functions

[PATCH net] xfrm: prevent some integer overflows in verify_ functions

[Dan Carpenter]

The xfrm_alg_len() type functions take the alg->alg_key_len which
is the length in bits and converts it to bytes and add it to
sizeof(*alg).

	return sizeof(*alg) + ((alg->alg_key_len + 7) / 8);

The alg->alg_key_len is type unsigned int.  That means that if we pick
an ->alg_key_len which is greater than "UINT_MAX - 7" it leads to an
integer overflow and the key length is treated as zero.  The result
is that xfrm_alg_len() function will return "sizeof(*alg) + 0".

However, so far as I can see this does not cause a problem.  All the
places which use this length consistently do the same conversion.  The
type of thing I was looking for would be code which uses partial keys
or code which uses a different type instead of u32 for ->alg_key_len.
I didn't find anything like that so I can't see a negative impact from
this bug.  Still fixing it is the right thing to do.

Fixes: 31c26852cb2a ("[IPSEC]: Verify key payload in verify_one_algo")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 net/xfrm/xfrm_user.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 08c6d6f0179f..686c6a24d92b 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -45,6 +45,10 @@ static int verify_one_alg(struct nlattr **attrs, enum xfrm_attr_type_t type,
 		return 0;
 
 	algp = nla_data(rt);
+	if (algp->alg_key_len > INT_MAX) {
+		NL_SET_ERR_MSG(extack, "Invalid AUTH/CRYPT/COMP key length");
+		return -EINVAL;
+	}
 	if (nla_len(rt) < (int)xfrm_alg_len(algp)) {
 		NL_SET_ERR_MSG(extack, "Invalid AUTH/CRYPT/COMP attribute length");
 		return -EINVAL;
@@ -75,6 +79,10 @@ static int verify_auth_trunc(struct nlattr **attrs,
 		return 0;
 
 	algp = nla_data(rt);
+	if (algp->alg_key_len > INT_MAX) {
+		NL_SET_ERR_MSG(extack, "Invalid AUTH_TRUNC key length");
+		return -EINVAL;
+	}
 	if (nla_len(rt) < (int)xfrm_alg_auth_len(algp)) {
 		NL_SET_ERR_MSG(extack, "Invalid AUTH_TRUNC attribute length");
 		return -EINVAL;
@@ -93,6 +101,10 @@ static int verify_aead(struct nlattr **attrs, struct netlink_ext_ack *extack)
 		return 0;
 
 	algp = nla_data(rt);
+	if (algp->alg_key_len > INT_MAX) {
+		NL_SET_ERR_MSG(extack, "Invalid AEAD key length");
+		return -EINVAL;
+	}
 	if (nla_len(rt) < (int)aead_len(algp)) {
 		NL_SET_ERR_MSG(extack, "Invalid AEAD attribute length");
 		return -EINVAL;
-- 
2.45.2

Re: [PATCH net] xfrm: prevent some integer overflows in verify_ functions

[Herbert Xu]

On Tue, Dec 17, 2024 at 11:42:31AM +0300, Dan Carpenter wrote:
>
> +	if (algp->alg_key_len > INT_MAX) {

Why not check for UINT_MAX - 7? INT_MAX seems a bit arbitrary.

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 net] xfrm: prevent some integer overflows in verify_ functions

[Dan Carpenter]

On Tue, Dec 17, 2024 at 08:03:38PM +0800, Herbert Xu wrote:
> On Tue, Dec 17, 2024 at 11:42:31AM +0300, Dan Carpenter wrote:
> >
> > +	if (algp->alg_key_len > INT_MAX) {
> 
> Why not check for UINT_MAX - 7? INT_MAX seems a bit arbitrary.
> 

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.

regards,
dan carpenter

[PATCH net] xfrm: Rewrite key length conversion to avoid overflows

[Herbert Xu]

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@linaro.org>
Signed-off-by: Herbert Xu <herbert@gondor.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@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net] xfrm: Rewrite key length conversion to avoid overflows

[Dan Carpenter]

On Wed, Dec 18, 2024 at 05:42:35PM +0800, Herbert Xu wrote:
> 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:
> 

I like it!  So obvious in retrospect.  Kees, Justin, this is probably a
good strategy for dealing with round_up() related integer overflows
generally.

overflows to zero:	(len + 7) / 8
      no overflow:	len / 8 + !!(len & 7)

> 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.

The length is capped in verify_one_alg() type functions:

	if (nla_len(rt) < (int)xfrm_alg_len(algp)) {

nla_len() is a USHRT_MAX so the rounded value can't be higher than that.

The (int) cast is unnecessary and confusing.  The condition should
probably flipped around so the untrusted part is on the left.

	if (xfrm_alg_len(algp) > nla_len(rt))
		return -EINVAL;

regards,
dan carpenter

Re: [PATCH net] xfrm: Rewrite key length conversion to avoid overflows

[Herbert Xu]

On Wed, Dec 18, 2024 at 01:54:38PM +0300, Dan Carpenter wrote:
>
> The length is capped in verify_one_alg() type functions:
> 
> 	if (nla_len(rt) < (int)xfrm_alg_len(algp)) {
> 
> nla_len() is a USHRT_MAX so the rounded value can't be higher than that.

Good catch.  I hope a similar limit applies for af_key?

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