diff mbox series

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

Message ID Z2KZC71JZ0QnrhfU@gondor.apana.org.au (mailing list archive)
State New
Delegated to: Netdev Maintainers
Headers show
Series [net] xfrm: Rewrite key length conversion to avoid overflows | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers warning 11 maintainers not CCed: borisp@nvidia.com saeedm@nvidia.com viro@zeniv.linux.org.uk leon@kernel.org dsahern@kernel.org oss-drivers@corigine.com louis.peens@corigine.com linux-rdma@vger.kernel.org tariqt@nvidia.com andrew+netdev@lunn.ch ayush.sawal@chelsio.com
netdev/build_clang success Errors and warnings before: 206 this patch: 206
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 222 this patch: 222
netdev/checkpatch warning WARNING: Possible repeated word: 'do' WARNING: line length of 83 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 95 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-12-18--12-00 (tests: 877)

Commit Message

Herbert Xu Dec. 18, 2024, 9:42 a.m. UTC 
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>

Comments

Dan Carpenter Dec. 18, 2024, 10:54 a.m. UTC | #1 
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
Herbert Xu Dec. 18, 2024, 11:58 a.m. UTC | #2 
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,
diff mbox series

Patch

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)