* [PATCH v2 1/2] af_key: Always verify length of provided sadb_key
2018-04-07 15:40 [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun Kevin Easton
@ 2018-04-07 15:40 ` Kevin Easton
2018-04-09 10:33 ` Steffen Klassert
2018-04-07 15:40 ` [PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent Kevin Easton
2018-04-09 10:32 ` [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun Steffen Klassert
2 siblings, 1 reply; 7+ messages in thread
From: Kevin Easton @ 2018-04-07 15:40 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu, David S. Miller, netdev,
linux-kernel
Key extensions (struct sadb_key) include a user-specified number of key
bits. The kernel uses that number to determine how much key data to copy
out of the message in pfkey_msg2xfrm_state().
The length of the sadb_key message must be verified to be long enough,
even in the case of SADB_X_AALG_NULL. Furthermore, the sadb_key_len value
must be long enough to include both the key data and the struct sadb_key
itself.
Introduce a helper function verify_key_len(), and call it from
parse_exthdrs() where other exthdr types are similarly checked for
correctness.
Signed-off-by: Kevin Easton <kevin@guarana.org>
Reported-by: syzbot+5022a34ca5a3d49b84223653fab632dfb7b4cf37@syzkaller.appspotmail.com
---
net/key/af_key.c | 45 +++++++++++++++++++++++++++++++++++----------
1 file changed, 35 insertions(+), 10 deletions(-)
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 7e2e718..e62e52e 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -437,6 +437,24 @@ static int verify_address_len(const void *p)
return 0;
}
+static inline int sadb_key_len(const struct sadb_key *key)
+{
+ int key_bytes = DIV_ROUND_UP(key->sadb_key_bits, 8);
+
+ return DIV_ROUND_UP(sizeof(struct sadb_key) + key_bytes,
+ sizeof(uint64_t));
+}
+
+static int verify_key_len(const void *p)
+{
+ const struct sadb_key *key = p;
+
+ if (sadb_key_len(key) > key->sadb_key_len)
+ return -EINVAL;
+
+ return 0;
+}
+
static inline int pfkey_sec_ctx_len(const struct sadb_x_sec_ctx *sec_ctx)
{
return DIV_ROUND_UP(sizeof(struct sadb_x_sec_ctx) +
@@ -533,16 +551,25 @@ static int parse_exthdrs(struct sk_buff *skb, const struct sadb_msg *hdr, void *
return -EINVAL;
if (ext_hdrs[ext_type-1] != NULL)
return -EINVAL;
- if (ext_type == SADB_EXT_ADDRESS_SRC ||
- ext_type == SADB_EXT_ADDRESS_DST ||
- ext_type == SADB_EXT_ADDRESS_PROXY ||
- ext_type == SADB_X_EXT_NAT_T_OA) {
+ switch (ext_type) {
+ case SADB_EXT_ADDRESS_SRC:
+ case SADB_EXT_ADDRESS_DST:
+ case SADB_EXT_ADDRESS_PROXY:
+ case SADB_X_EXT_NAT_T_OA:
if (verify_address_len(p))
return -EINVAL;
- }
- if (ext_type == SADB_X_EXT_SEC_CTX) {
+ break;
+ case SADB_X_EXT_SEC_CTX:
if (verify_sec_ctx_len(p))
return -EINVAL;
+ break;
+ case SADB_EXT_KEY_AUTH:
+ case SADB_EXT_KEY_ENCRYPT:
+ if (verify_key_len(p))
+ return -EINVAL;
+ break;
+ default:
+ break;
}
ext_hdrs[ext_type-1] = (void *) p;
}
@@ -1104,14 +1131,12 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
key = ext_hdrs[SADB_EXT_KEY_AUTH - 1];
if (key != NULL &&
sa->sadb_sa_auth != SADB_X_AALG_NULL &&
- ((key->sadb_key_bits+7) / 8 == 0 ||
- (key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+ key->sadb_key_bits == 0)
return ERR_PTR(-EINVAL);
key = ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
if (key != NULL &&
sa->sadb_sa_encrypt != SADB_EALG_NULL &&
- ((key->sadb_key_bits+7) / 8 == 0 ||
- (key->sadb_key_bits+7) / 8 > key->sadb_key_len * sizeof(uint64_t)))
+ key->sadb_key_bits == 0)
return ERR_PTR(-EINVAL);
x = xfrm_state_alloc(net);
--
2.8.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent
2018-04-07 15:40 [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun Kevin Easton
2018-04-07 15:40 ` [PATCH v2 1/2] af_key: Always verify length of provided sadb_key Kevin Easton
@ 2018-04-07 15:40 ` Kevin Easton
2018-04-09 10:34 ` Steffen Klassert
2018-04-09 10:32 ` [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun Steffen Klassert
2 siblings, 1 reply; 7+ messages in thread
From: Kevin Easton @ 2018-04-07 15:40 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu, David S. Miller, netdev,
linux-kernel
Several places use (x + 7) / 8 to convert from a number of bits to a number
of bytes. Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
with other parts of the same file.
Signed-off-by: Kevin Easton <kevin@guarana.org>
---
net/key/af_key.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/key/af_key.c b/net/key/af_key.c
index e62e52e..f3ebb84 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -822,12 +822,12 @@ 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);
+ PFKEY_ALIGN8(DIV_ROUND_UP(x->aalg->alg_key_len, 8));
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);
+ PFKEY_ALIGN8(DIV_ROUND_UP(x->ealg->alg_key_len, 8));
size += sizeof(struct sadb_key) + encrypt_key_size;
}
}
@@ -987,7 +987,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,
+ DIV_ROUND_UP(x->aalg->alg_key_len, 8));
}
/* encrypt key */
if (add_keys && encrypt_key_size) {
@@ -998,7 +999,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);
+ DIV_ROUND_UP(x->ealg->alg_key_len, 8));
}
/* sa */
@@ -1193,7 +1194,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
goto out;
}
if (key)
- keysize = (key->sadb_key_bits + 7) / 8;
+ keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
x->aalg = kmalloc(sizeof(*x->aalg) + keysize, GFP_KERNEL);
if (!x->aalg) {
err = -ENOMEM;
@@ -1232,7 +1233,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
}
key = (struct sadb_key*) ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
if (key)
- keysize = (key->sadb_key_bits + 7) / 8;
+ keysize = DIV_ROUND_UP(key->sadb_key_bits, 8);
x->ealg = kmalloc(sizeof(*x->ealg) + keysize, GFP_KERNEL);
if (!x->ealg) {
err = -ENOMEM;
--
2.8.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun
2018-04-07 15:40 [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun Kevin Easton
2018-04-07 15:40 ` [PATCH v2 1/2] af_key: Always verify length of provided sadb_key Kevin Easton
2018-04-07 15:40 ` [PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent Kevin Easton
@ 2018-04-09 10:32 ` Steffen Klassert
2 siblings, 0 replies; 7+ messages in thread
From: Steffen Klassert @ 2018-04-09 10:32 UTC (permalink / raw)
To: Kevin Easton; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
On Sat, Apr 07, 2018 at 11:40:18AM -0400, Kevin Easton wrote:
> As found by syzbot, af_key does not properly validate the key length in
> sadb_key messages from userspace. This can result in copying from beyond
> the end of the sadb_key part of the message, or indeed beyond the end of
> the entire packet.
>
> Both these patches apply cleanly to ipsec-next. Based on Steffen's
> feedback I have re-ordered them so that the fix only is in patch 1, which
> I would suggest is also a stable tree candidate, whereas patch 2 is a
> cleanup only.
I think here is some explanation needed. Usually bugfixes and cleanups
don't go to the same tree. On IPsec bugfixes go to the'ipsec' tree
while cleanups and new features go to the 'ipsec-next' tree. So
you need to split up your patchsets into patches that are targeted
to 'ipsec' and 'ipsec-next'. Aside from that, we are in 'merge window'
currently. This means that most maintainers don't accept patches to
their -next trees. If you have patches for a -next tree, wait until
the merge window is over (when v4.17-rc1 is released) and send them
then.
^ permalink raw reply [flat|nested] 7+ messages in thread