From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steffen Klassert Subject: Re: Possible fix Date: Fri, 28 Feb 2014 08:23:33 +0100 Message-ID: <20140228072333.GP32371@secunet.com> References: <20140227151954.GA30946@redhat.com> <1393517857-11842-1-git-send-email-nikolay@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: , Dave Jones , Fan Du , "David S. Miller" , Paul Moore , To: Nikolay Aleksandrov Return-path: Content-Disposition: inline In-Reply-To: <1393517857-11842-1-git-send-email-nikolay@redhat.com> Sender: linux-security-module-owner@vger.kernel.org List-Id: netdev.vger.kernel.org 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()). > CC: Dave Jones > CC: Steffen Klassert > CC: Fan Du > CC: David S. Miller > > Signed-off-by: Nikolay Aleksandrov > --- > 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.