Re: Possible fix
From: Steffen Klassert <steffen.klassert@secunet.com>
Date: 2014-02-28 07:23:33
Ccing some security/selinux people. On Thu, Feb 27, 2014 at 05:17:37PM +0100, Nikolay Aleksandrov wrote:
Hi, I'm not familiar with the code but happened to see the bug, could you try the following patch, I believe it should fix the issue. Thanks, Nik [PATCH net] net: af_key: fix sleeping under rcu There's a kmalloc with GFP_KERNEL in a helper (pfkey_sadb2xfrm_user_sec_ctx) used in pfkey_compile_policy which is called under rcu_read_lock. Adjust pfkey_sadb2xfrm_user_sec_ctx to have a gfp argument and adjust the users.
Looking at the git history, it seems that this bug is about nine years old. I guess noone is actually using this. Also, we care for the security context only if we add a socket policy via the pfkey key manager. The security context is not handled if we do that with the netlink key manager (compare pfkey_compile_policy() and xfrm_compile_policy()).
quoted hunk ↗ jump to hunk
CC: Dave Jones <redacted> CC: Steffen Klassert <steffen.klassert@secunet.com> CC: Fan Du <redacted> CC: David S. Miller <davem@davemloft.net> Signed-off-by: Nikolay Aleksandrov <redacted> --- I'm not familiar with this code, but just happen to see the bug. I believe this patch should take care of it. I've left the already very long lines. 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 1a04c1329362..1526023f99ed 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c@@ -433,12 +433,13 @@ static inline int verify_sec_ctx_len(const void *p) return 0; } -static inline struct xfrm_user_sec_ctx *pfkey_sadb2xfrm_user_sec_ctx(const struct sadb_x_sec_ctx *sec_ctx) +static inline struct xfrm_user_sec_ctx *pfkey_sadb2xfrm_user_sec_ctx(const struct sadb_x_sec_ctx *sec_ctx, + gfp_t gfp) { struct xfrm_user_sec_ctx *uctx = NULL; int ctx_size = sec_ctx->sadb_x_ctx_len; - uctx = kmalloc((sizeof(*uctx)+ctx_size), GFP_KERNEL); + uctx = kmalloc((sizeof(*uctx)+ctx_size), gfp); if (!uctx) return NULL;@@ -1124,7 +1125,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net, sec_ctx = ext_hdrs[SADB_X_EXT_SEC_CTX - 1]; if (sec_ctx != NULL) { - struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); + struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx, GFP_KERNEL); if (!uctx) goto out;@@ -2231,7 +2232,7 @@ static int pfkey_spdadd(struct sock *sk, struct sk_buff *skb, const struct sadb_ sec_ctx = ext_hdrs[SADB_X_EXT_SEC_CTX - 1]; if (sec_ctx != NULL) { - struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); + struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx, GFP_KERNEL); if (!uctx) { err = -ENOBUFS;@@ -2335,7 +2336,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa sec_ctx = ext_hdrs[SADB_X_EXT_SEC_CTX - 1]; if (sec_ctx != NULL) { - struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); + struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx, GFP_KERNEL); if (!uctx) return -ENOMEM;@@ -3239,7 +3240,7 @@ static struct xfrm_policy *pfkey_compile_policy(struct sock *sk, int opt, } if ((*dir = verify_sec_ctx_len(p))) goto out; - uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); + uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx, GFP_ATOMIC); *dir = security_xfrm_policy_alloc(&xp->security, uctx);
This would fix the allocation done in pfkey_sadb2xfrm_user_sec_ctx(). But security_xfrm_policy_alloc() might call selinux_xfrm_alloc_user() which does a GFP_KERNEL allocation too. So I guess we also need to fix selinux.