On 02/28/2014 11:10 PM, Paul Moore wrote:

On Friday, February 28, 2014 11:10:07 AM Nikolay Aleksandrov wrote:

quoted

On 02/28/2014 08:23 AM, Steffen Klassert wrote:

quoted

On Thu, Feb 27, 2014 at 05:17:37PM +0100, Nikolay Aleksandrov wrote:

quoted

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.

Most (all?) of the labeled IPsec users use the netlink interface and not pfkey 
so it isn't surprising that this has gone unnoticed for some time.

quoted

quoted

quoted

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

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

Yes, exactly.

quoted

Right, I just saw that but fixing it at first glance doesn't seem so
trivial as we can't pass another argument from compile_policy without
changing xfrm_policy_alloc_security's prototype in struct
security_operations which AFAICT is doable with some adjustments, but not
sure if it's the right thing to do. Changing GFP_KERNEL to GFP_ATOMIC in
selinux_xfrm_alloc_user is also a possibility, but there're a many places
which use that and can sleep.

I would recommend adding a gfp_t argument to security_xfrm_policy_alloc() and 
passing GFP_ATOMIC in pfkey_compile_policy().

Okay, will do.

quoted

I would extend this patch, but currently don't have the time to search for
a nice solution. I can look more into it next week, or if you'd like to
take care of it, I wouldn't mind :-)

It has been this way for a while so I think another day or two isn't going to 
cause any major harm.  If you are going to put a patch together that's great, 
CC me and I'll review/ACK it, but if you don't want to bother let me know and 
I'll work on a patch.

Thanks,
-Paul

I'll fix up the patch then and re-submit properly.

Cheers,
 Nik