kmalloc with locks held in xfrm.

kmalloc with locks held in xfrm.

[Dave Jones]

Hit this trace this morning on rc4.

	Dave

BUG: sleeping function called from invalid context at mm/slub.c:965
in_atomic(): 0, irqs_disabled(): 0, pid: 28692, name: trinity-main
2 locks held by trinity-main/28692:
 #0:  (sk_lock-AF_INET){......}, at: [<ffffffff83660cd3>] do_ip_setsockopt.isra.12+0x63/0xe10
 #1:  (rcu_read_lock){......}, at: [<ffffffff836bf138>] xfrm_user_policy+0xa8/0x1c0
CPU: 0 PID: 28692 Comm: trinity-main Not tainted 3.14.0-rc4+ #125 
 ffffffff83a428e7 00000000707766bf ffff88008c749c50 ffffffff8372e248
 ffff8800075c45c0 ffff88008c749c80 ffffffff8309c525 00000000000000d0
 00000000000000d0 0000000000000008 ffff880244c04240 ffff88008c749cd8
Call Trace:
 [<ffffffff8372e248>] dump_stack+0x4e/0x7a
 [<ffffffff8309c525>] __might_sleep+0x125/0x180
 [<ffffffff831acd6a>] __kmalloc+0x7a/0x2a0
 [<ffffffffc05978d8>] ? pfkey_compile_policy+0x1a8/0x270 [af_key]
 [<ffffffffc05978d8>] pfkey_compile_policy+0x1a8/0x270 [af_key]
 [<ffffffff836bf1b7>] xfrm_user_policy+0x127/0x1c0
 [<ffffffff836bf138>] ? xfrm_user_policy+0xa8/0x1c0
 [<ffffffff83071907>] ? __local_bh_enable_ip+0x67/0xe0
 [<ffffffff8366173e>] do_ip_setsockopt.isra.12+0xace/0xe10
 [<ffffffff831c1e7c>] ? get_empty_filp+0x5c/0x240
 [<ffffffff830bcab5>] ? cpuacct_account_field+0x5/0xc0
 [<ffffffff8310ff8e>] ? __acct_update_integrals+0x8e/0x120
 [<ffffffff830a388d>] ? get_parent_ip+0xd/0x50
 [<ffffffff8373d42b>] ? preempt_count_sub+0x6b/0xf0
 [<ffffffff83661ab0>] ip_setsockopt+0x30/0xa0
 [<ffffffff836872eb>] udp_setsockopt+0x1b/0x40
 [<ffffffff83600dc4>] sock_common_setsockopt+0x14/0x20
 [<ffffffff835ffa30>] SyS_setsockopt+0x80/0xf0
 [<ffffffff83741e6a>] tracesys+0xd4/0xd9

Possible fix

[Nikolay Aleksandrov]

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.

CC: Dave Jones <davej@redhat.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Fan Du <fan.du@windriver.com>
CC: David S. Miller <davem@davemloft.net>

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
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);
 		kfree(uctx);
 
-- 
1.8.5.3

Re: Possible fix

[Nikolay Aleksandrov]

On 02/27/2014 05:17 PM, 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.
> 
> CC: Dave Jones <davej@redhat.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Fan Du <fan.du@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
> 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.
> 
Actually there isn't a check if uctx is NULL upon return which can happen.
This is only a test patch and that check should be added as well.

Nik

Re: Possible fix

[Nikolay Aleksandrov]

----- Original Message -----
From: "Nikolay Aleksandrov" <nikolay@redhat.com>
To: netdev@vger.kernel.org
Cc: "Nikolay Aleksandrov" <nikolay@redhat.com>, "Dave Jones" <davej@redhat.com>, "Steffen Klassert" <steffen.klassert@secunet.com>, "Fan Du" <fan.du@windriver.com>, "David S. Miller" <davem@davemloft.net>
Sent: Thursday, February 27, 2014 5:24:30 PM
Subject: Re: Possible fix

On 02/27/2014 05:17 PM, 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.
> 
> CC: Dave Jones <davej@redhat.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Fan Du <fan.du@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
> 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.
> 
>Actually there isn't a check if uctx is NULL upon return which can happen.
>This is only a test patch and that check should be added as well.
>
>Nik
Had a quick look and I think that should be okay, there isn't a need to check.

Sent from my phone

Re: Possible fix

[Steffen Klassert]

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 <davej@redhat.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Fan Du <fan.du@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> ---
> 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.

Re: Possible fix

[Nikolay Aleksandrov]

On 02/28/2014 08:23 AM, Steffen Klassert wrote:
> 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 <davej@redhat.com>
>> CC: Steffen Klassert <steffen.klassert@secunet.com>
>> CC: Fan Du <fan.du@windriver.com>
>> CC: David S. Miller <davem@davemloft.net>
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>> ---
>> 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.
> 
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 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 :-)

Cheers,
 Nik

Re: Possible fix

[Paul Moore]

On Friday, February 28, 2014 11:10:07 AM Nikolay Aleksandrov wrote:
> On 02/28/2014 08:23 AM, Steffen Klassert wrote:
> > 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.

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.

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

> 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().

> 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

-- 
paul moore
www.paul-moore.com

Re: Possible fix

[Nikolay Aleksandrov]

On 02/28/2014 11:10 PM, Paul Moore wrote:
> On Friday, February 28, 2014 11:10:07 AM Nikolay Aleksandrov wrote:
>> On 02/28/2014 08:23 AM, Steffen Klassert wrote:
>>> 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.
> 
> 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.
> 
>>>> 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.
> 
>> 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.

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

Re: Possible fix

[Steffen Klassert]

On Fri, Feb 28, 2014 at 05:10:47PM -0500, Paul Moore wrote:
> On Friday, February 28, 2014 11:10:07 AM Nikolay Aleksandrov wrote:
> > On 02/28/2014 08:23 AM, Steffen Klassert wrote:
> > > 
> > > 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.

Right, that's not really surprising. But it is a bit surprising that
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, see xfrm_compile_policy().

I'm not that familiar with selinux and labeled IPsec, but maybe this
needs to be implemented in xfrm_compile_policy() too.

Re: Possible fix

[Paul Moore]

On Wednesday, March 05, 2014 01:20:09 PM Steffen Klassert wrote:
> On Fri, Feb 28, 2014 at 05:10:47PM -0500, Paul Moore wrote:
> > On Friday, February 28, 2014 11:10:07 AM Nikolay Aleksandrov wrote:
> > > On 02/28/2014 08:23 AM, Steffen Klassert wrote:
> > > > 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.
>
> Right, that's not really surprising. But it is a bit surprising that
> 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, see xfrm_compile_policy().
> 
> I'm not that familiar with selinux and labeled IPsec, but maybe this
> needs to be implemented in xfrm_compile_policy() too.

Okay, I see your point.  We probably should add support for per-socket policy 
labels just to keep parity with the pfkey code (and this is far removed from 
any critical path), but to be honest it isn't something that I think would get 
much use in practice.  Labeled networking users tend to fall under the very 
strict, one-system-wide-security-policy and per-socket policies tend to go 
against that logic.

I'll have to think about it some more.

-- 
paul moore
www.paul-moore.com

Re: Possible fix

[Steffen Klassert]

On Thu, Mar 06, 2014 at 10:04:54PM -0500, Paul Moore wrote:
> On Wednesday, March 05, 2014 01:20:09 PM Steffen Klassert wrote:
> >
> > Right, that's not really surprising. But it is a bit surprising that
> > 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, see xfrm_compile_policy().
> > 
> > I'm not that familiar with selinux and labeled IPsec, but maybe this
> > needs to be implemented in xfrm_compile_policy() too.
> 
> Okay, I see your point.  We probably should add support for per-socket policy 
> labels just to keep parity with the pfkey code (and this is far removed from 
> any critical path), but to be honest it isn't something that I think would get 
> much use in practice.  Labeled networking users tend to fall under the very 
> strict, one-system-wide-security-policy and per-socket policies tend to go 
> against that logic.
> 

If you think socket policy labels are no usecase for labeled IPsec, we could
fix this bug simply by removing the code from pfkey ;)

Otherwise I think we should implement it for xfrm_compile_policy() too.

Re: Possible fix

[Paul Moore]

On Friday, March 07, 2014 12:23:34 PM Steffen Klassert wrote:
> On Thu, Mar 06, 2014 at 10:04:54PM -0500, Paul Moore wrote:
> > On Wednesday, March 05, 2014 01:20:09 PM Steffen Klassert wrote:
> > > Right, that's not really surprising. But it is a bit surprising that
> > > 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, see xfrm_compile_policy().
> > > 
> > > I'm not that familiar with selinux and labeled IPsec, but maybe this
> > > needs to be implemented in xfrm_compile_policy() too.
> > 
> > Okay, I see your point.  We probably should add support for per-socket
> > policy labels just to keep parity with the pfkey code (and this is far
> > removed from any critical path), but to be honest it isn't something that
> > I think would get much use in practice.  Labeled networking users tend to
> > fall under the very strict, one-system-wide-security-policy and
> > per-socket policies tend to go against that logic.
> 
> If you think socket policy labels are no usecase for labeled IPsec, we could
> fix this bug simply by removing the code from pfkey ;)
> 
> Otherwise I think we should implement it for xfrm_compile_policy() too.

In general I dislike removing functionality/capability so I'm inclined to add 
support to xfrm_compile_policy and call it good.

-- 
paul moore
www.paul-moore.com

[PATCH 0/2] af_key: fixes for sleeping while atomic

[Nikolay Aleksandrov]

Hi,
The first patch takes care of the issue on the af_key side, and the second
one fixes it on the selinux side (security_xfrm_policy_alloc).
There're a few lines >80 but they were already long, I'm not sure if breaking
them above the limit would make them more readable than they already are.

Best regards,
 Nikolay Aleksandrov

CC: Paul Moore <paul@paul-moore.com>
CC: Dave Jones <davej@redhat.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Fan Du <fan.du@windriver.com>
CC: David S. Miller <davem@davemloft.net>

Nikolay Aleksandrov (2):
  net: af_key: fix sleeping under rcu
  selinux: add gfp argument to security_xfrm_policy_alloc and fix
    callers

 include/linux/security.h        | 10 +++++++---
 net/key/af_key.c                | 19 ++++++++++---------
 net/xfrm/xfrm_user.c            |  6 +++---
 security/capability.c           |  3 ++-
 security/security.c             |  6 ++++--
 security/selinux/include/xfrm.h |  3 ++-
 security/selinux/xfrm.c         | 12 +++++++-----
 7 files changed, 35 insertions(+), 24 deletions(-)

-- 
1.8.5.3

[PATCH 1/2] net: af_key: fix sleeping under rcu

[Nikolay Aleksandrov]

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.

CC: Dave Jones <davej@redhat.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Fan Du <fan.du@windriver.com>
CC: David S. Miller <davem@davemloft.net>

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 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);
 		kfree(uctx);
 
-- 
1.8.5.3

RE: [PATCH 1/2] net: af_key: fix sleeping under rcu

[David Laight]

From: Nikolay Aleksandrov
> 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.
...
> @@ -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);
>  		kfree(uctx);

This looks like the only one that isn't passed GFP_KERNEL.
It looks as though it is missing the check for the allocation failing
(there might be a check inside security_xfrm_policy_alloc()).

In any case it looks as though this ought to be codeable without
the allocation of 'uctx' - since it is freed a line later.

I'd have thought that some of these paths should be allocating
the memory outside the rcu (where they can sleep) and then
possibly freeing the unused data later.

Returning ENOMEM doesn't really seem helpful.

	David

Re: [PATCH 1/2] net: af_key: fix sleeping under rcu

[David Miller]

From: David Laight <David.Laight@ACULAB.COM>
Date: Tue, 4 Mar 2014 12:46:48 +0000

> From: Nikolay Aleksandrov
>> 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.
> ...
>> @@ -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);
>>  		kfree(uctx);
> 
> This looks like the only one that isn't passed GFP_KERNEL.
> It looks as though it is missing the check for the allocation failing
> (there might be a check inside security_xfrm_policy_alloc()).
> 
> In any case it looks as though this ought to be codeable without
> the allocation of 'uctx' - since it is freed a line later.

Unfortunately, it is not possible to avoid allocations.  The uctx is
of a variable size, because it is a base struct, with a variable
length part afterwards.

[PATCH 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers

[Nikolay Aleksandrov]

security_xfrm_policy_alloc can be called in atomic context so the
allocation should be done with GFP_ATOMIC. Add an argument to let the
callers choose the appropriate way. In order to do so a gfp argument
needs to be added to the method xfrm_policy_alloc_security in struct
security_operations and to the internal function
selinux_xfrm_alloc_user. After that switch to GFP_ATOMIC in the atomic
callers and leave GFP_KERNEL as before for the rest.
The path that needed the gfp argument addition is:
security_xfrm_policy_alloc -> security_ops.xfrm_policy_alloc_security ->
all users of xfrm_policy_alloc_security (e.g. selinux_xfrm_policy_alloc) ->
selinux_xfrm_alloc_user (here the allocation used to be GFP_KERNEL only)

CC: Paul Moore <paul@paul-moore.com>
CC: Dave Jones <davej@redhat.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Fan Du <fan.du@windriver.com>
CC: David S. Miller <davem@davemloft.net>
CC: LSM list <linux-security-module@vger.kernel.org>

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 include/linux/security.h        | 10 +++++++---
 net/key/af_key.c                |  6 +++---
 net/xfrm/xfrm_user.c            |  6 +++---
 security/capability.c           |  3 ++-
 security/security.c             |  6 ++++--
 security/selinux/include/xfrm.h |  3 ++-
 security/selinux/xfrm.c         | 12 +++++++-----
 7 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 5623a7f965b7..2fc42d191f79 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1040,6 +1040,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	Allocate a security structure to the xp->security field; the security
  *	field is initialized to NULL when the xfrm_policy is allocated.
  *	Return 0 if operation was successful (memory to allocate, legal context)
+ *	@gfp is to specify the context for the allocation
  * @xfrm_policy_clone_security:
  *	@old_ctx contains an existing xfrm_sec_ctx.
  *	@new_ctxp contains a new xfrm_sec_ctx being cloned from old.
@@ -1683,7 +1684,7 @@ struct security_operations {
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	int (*xfrm_policy_alloc_security) (struct xfrm_sec_ctx **ctxp,
-			struct xfrm_user_sec_ctx *sec_ctx);
+			struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp);
 	int (*xfrm_policy_clone_security) (struct xfrm_sec_ctx *old_ctx, struct xfrm_sec_ctx **new_ctx);
 	void (*xfrm_policy_free_security) (struct xfrm_sec_ctx *ctx);
 	int (*xfrm_policy_delete_security) (struct xfrm_sec_ctx *ctx);
@@ -2859,7 +2860,8 @@ static inline void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 
-int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct xfrm_user_sec_ctx *sec_ctx);
+int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
+			       struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp);
 int security_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx, struct xfrm_sec_ctx **new_ctxp);
 void security_xfrm_policy_free(struct xfrm_sec_ctx *ctx);
 int security_xfrm_policy_delete(struct xfrm_sec_ctx *ctx);
@@ -2877,7 +2879,9 @@ void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl);
 
 #else	/* CONFIG_SECURITY_NETWORK_XFRM */
 
-static inline int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct xfrm_user_sec_ctx *sec_ctx)
+static inline int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
+					     struct xfrm_user_sec_ctx *sec_ctx,
+					     gfp_t gfp)
 {
 	return 0;
 }
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 1526023f99ed..79326978517a 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2239,7 +2239,7 @@ static int pfkey_spdadd(struct sock *sk, struct sk_buff *skb, const struct sadb_
 			goto out;
 		}
 
-		err = security_xfrm_policy_alloc(&xp->security, uctx);
+		err = security_xfrm_policy_alloc(&xp->security, uctx, GFP_KERNEL);
 		kfree(uctx);
 
 		if (err)
@@ -2341,7 +2341,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
 		if (!uctx)
 			return -ENOMEM;
 
-		err = security_xfrm_policy_alloc(&pol_ctx, uctx);
+		err = security_xfrm_policy_alloc(&pol_ctx, uctx, GFP_KERNEL);
 		kfree(uctx);
 		if (err)
 			return err;
@@ -3241,7 +3241,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, GFP_ATOMIC);
-		*dir = security_xfrm_policy_alloc(&xp->security, uctx);
+		*dir = security_xfrm_policy_alloc(&xp->security, uctx, GFP_ATOMIC);
 		kfree(uctx);
 
 		if (*dir)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 1ae3ec7c18b0..7c8278e55bde 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1226,7 +1226,7 @@ static int copy_from_user_sec_ctx(struct xfrm_policy *pol, struct nlattr **attrs
 		return 0;
 
 	uctx = nla_data(rt);
-	return security_xfrm_policy_alloc(&pol->security, uctx);
+	return security_xfrm_policy_alloc(&pol->security, uctx, GFP_KERNEL);
 }
 
 static void copy_templates(struct xfrm_policy *xp, struct xfrm_user_tmpl *ut,
@@ -1631,7 +1631,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 		if (rt) {
 			struct xfrm_user_sec_ctx *uctx = nla_data(rt);
 
-			err = security_xfrm_policy_alloc(&ctx, uctx);
+			err = security_xfrm_policy_alloc(&ctx, uctx, GFP_KERNEL);
 			if (err)
 				return err;
 		}
@@ -1933,7 +1933,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 		if (rt) {
 			struct xfrm_user_sec_ctx *uctx = nla_data(rt);
 
-			err = security_xfrm_policy_alloc(&ctx, uctx);
+			err = security_xfrm_policy_alloc(&ctx, uctx, GFP_KERNEL);
 			if (err)
 				return err;
 		}
diff --git a/security/capability.c b/security/capability.c
index 8b4f24ae4338..21e2b9cae685 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -757,7 +757,8 @@ static void cap_skb_owned_by(struct sk_buff *skb, struct sock *sk)
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 static int cap_xfrm_policy_alloc_security(struct xfrm_sec_ctx **ctxp,
-					  struct xfrm_user_sec_ctx *sec_ctx)
+					  struct xfrm_user_sec_ctx *sec_ctx,
+					  gfp_t gfp)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index 15b6928592ef..919cad93ac82 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1317,9 +1317,11 @@ void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 
-int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct xfrm_user_sec_ctx *sec_ctx)
+int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
+			       struct xfrm_user_sec_ctx *sec_ctx,
+			       gfp_t gfp)
 {
-	return security_ops->xfrm_policy_alloc_security(ctxp, sec_ctx);
+	return security_ops->xfrm_policy_alloc_security(ctxp, sec_ctx, gfp);
 }
 EXPORT_SYMBOL(security_xfrm_policy_alloc);
 
diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h
index 48c3cc94c168..9f0584710c85 100644
--- a/security/selinux/include/xfrm.h
+++ b/security/selinux/include/xfrm.h
@@ -10,7 +10,8 @@
 #include <net/flow.h>
 
 int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
-			      struct xfrm_user_sec_ctx *uctx);
+			      struct xfrm_user_sec_ctx *uctx,
+			      gfp_t gfp);
 int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
 			      struct xfrm_sec_ctx **new_ctxp);
 void selinux_xfrm_policy_free(struct xfrm_sec_ctx *ctx);
diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index 0462cb3ff0a7..7ae773f4fe38 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -78,7 +78,8 @@ static inline int selinux_authorizable_xfrm(struct xfrm_state *x)
  * xfrm_user_sec_ctx context.
  */
 static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
-				   struct xfrm_user_sec_ctx *uctx)
+				   struct xfrm_user_sec_ctx *uctx,
+				   gfp_t gfp)
 {
 	int rc;
 	const struct task_security_struct *tsec = current_security();
@@ -94,7 +95,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
 	if (str_len >= PAGE_SIZE)
 		return -ENOMEM;
 
-	ctx = kmalloc(sizeof(*ctx) + str_len + 1, GFP_KERNEL);
+	ctx = kmalloc(sizeof(*ctx) + str_len + 1, gfp);
 	if (!ctx)
 		return -ENOMEM;
 
@@ -282,9 +283,10 @@ int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid)
  * LSM hook implementation that allocs and transfers uctx spec to xfrm_policy.
  */
 int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
-			      struct xfrm_user_sec_ctx *uctx)
+			      struct xfrm_user_sec_ctx *uctx,
+			      gfp_t gfp)
 {
-	return selinux_xfrm_alloc_user(ctxp, uctx);
+	return selinux_xfrm_alloc_user(ctxp, uctx, gfp);
 }
 
 /*
@@ -332,7 +334,7 @@ int selinux_xfrm_policy_delete(struct xfrm_sec_ctx *ctx)
 int selinux_xfrm_state_alloc(struct xfrm_state *x,
 			     struct xfrm_user_sec_ctx *uctx)
 {
-	return selinux_xfrm_alloc_user(&x->security, uctx);
+	return selinux_xfrm_alloc_user(&x->security, uctx, GFP_KERNEL);
 }
 
 /*
-- 
1.8.5.3

Re: [PATCH 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers

[Paul Moore]

On Tuesday, March 04, 2014 01:26:24 PM Nikolay Aleksandrov wrote:
> security_xfrm_policy_alloc can be called in atomic context so the
> allocation should be done with GFP_ATOMIC. Add an argument to let the
> callers choose the appropriate way. In order to do so a gfp argument
> needs to be added to the method xfrm_policy_alloc_security in struct
> security_operations and to the internal function
> selinux_xfrm_alloc_user. After that switch to GFP_ATOMIC in the atomic
> callers and leave GFP_KERNEL as before for the rest.
> The path that needed the gfp argument addition is:
> security_xfrm_policy_alloc -> security_ops.xfrm_policy_alloc_security ->
> all users of xfrm_policy_alloc_security (e.g. selinux_xfrm_policy_alloc) ->
> selinux_xfrm_alloc_user (here the allocation used to be GFP_KERNEL only)
> 
> CC: Paul Moore <paul@paul-moore.com>
> CC: Dave Jones <davej@redhat.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Fan Du <fan.du@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: LSM list <linux-security-module@vger.kernel.org>
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

[NOTE: added the SELinux list to the CC list above]

In general, the patch is pretty simple with the obvious necessary changes, 
just one gotcha, see below.

> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 0462cb3ff0a7..7ae773f4fe38 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -78,7 +78,8 @@ static inline int selinux_authorizable_xfrm(struct
> xfrm_state *x) * xfrm_user_sec_ctx context.
>   */
>  static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
> -				   struct xfrm_user_sec_ctx *uctx)
> +				   struct xfrm_user_sec_ctx *uctx,
> +				   gfp_t gfp)
>  {
>  	int rc;
>  	const struct task_security_struct *tsec = current_security();
> @@ -94,7 +95,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx
> **ctxp, if (str_len >= PAGE_SIZE)
>  		return -ENOMEM;
> 
> -	ctx = kmalloc(sizeof(*ctx) + str_len + 1, GFP_KERNEL);
> +	ctx = kmalloc(sizeof(*ctx) + str_len + 1, gfp);
>  	if (!ctx)
>  		return -ENOMEM;

Also located in selinux_xfrm_alloc_user() is a call to 
security_context_to_sid() which calls security_context_to_sid_core() which in 
some cases does allocate memory.  The good news is that to_sid_core() does 
accept a gfp_t flag, the bad news is that to_sid() always passes GFP_KERNEL.

It looks like we need to extend this patch a bit, or add another.  Sorry about 
that.  If you're getting tired of playing with the LSM/SELinux code let me 
know :)

> @@ -282,9 +283,10 @@ int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid)
> * LSM hook implementation that allocs and transfers uctx spec to
> xfrm_policy. */
>  int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
> -			      struct xfrm_user_sec_ctx *uctx)
> +			      struct xfrm_user_sec_ctx *uctx,
> +			      gfp_t gfp)
>  {
> -	return selinux_xfrm_alloc_user(ctxp, uctx);
> +	return selinux_xfrm_alloc_user(ctxp, uctx, gfp);
>  }
> 
>  /*
> @@ -332,7 +334,7 @@ int selinux_xfrm_policy_delete(struct xfrm_sec_ctx *ctx)
> int selinux_xfrm_state_alloc(struct xfrm_state *x,
>  			     struct xfrm_user_sec_ctx *uctx)
>  {
> -	return selinux_xfrm_alloc_user(&x->security, uctx);
> +	return selinux_xfrm_alloc_user(&x->security, uctx, GFP_KERNEL);
>  }
> 
>  /*

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers

[Nikolay Aleksandrov]

On 03/07/2014 04:22 AM, Paul Moore wrote:
> On Tuesday, March 04, 2014 01:26:24 PM Nikolay Aleksandrov wrote:
>> security_xfrm_policy_alloc can be called in atomic context so the
>> allocation should be done with GFP_ATOMIC. Add an argument to let the
>> callers choose the appropriate way. In order to do so a gfp argument
>> needs to be added to the method xfrm_policy_alloc_security in struct
>> security_operations and to the internal function
>> selinux_xfrm_alloc_user. After that switch to GFP_ATOMIC in the atomic
>> callers and leave GFP_KERNEL as before for the rest.
>> The path that needed the gfp argument addition is:
>> security_xfrm_policy_alloc -> security_ops.xfrm_policy_alloc_security ->
>> all users of xfrm_policy_alloc_security (e.g. selinux_xfrm_policy_alloc) ->
>> selinux_xfrm_alloc_user (here the allocation used to be GFP_KERNEL only)
>>
>> CC: Paul Moore <paul@paul-moore.com>
>> CC: Dave Jones <davej@redhat.com>
>> CC: Steffen Klassert <steffen.klassert@secunet.com>
>> CC: Fan Du <fan.du@windriver.com>
>> CC: David S. Miller <davem@davemloft.net>
>> CC: LSM list <linux-security-module@vger.kernel.org>
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> 
> [NOTE: added the SELinux list to the CC list above]
> 
> In general, the patch is pretty simple with the obvious necessary changes, 
> just one gotcha, see below.
> 
Thanks, I'll add the SELinux list to the CCs next time.

>> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
>> index 0462cb3ff0a7..7ae773f4fe38 100644
>> --- a/security/selinux/xfrm.c
>> +++ b/security/selinux/xfrm.c
>> @@ -78,7 +78,8 @@ static inline int selinux_authorizable_xfrm(struct
>> xfrm_state *x) * xfrm_user_sec_ctx context.
>>   */
>>  static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
>> -				   struct xfrm_user_sec_ctx *uctx)
>> +				   struct xfrm_user_sec_ctx *uctx,
>> +				   gfp_t gfp)
>>  {
>>  	int rc;
>>  	const struct task_security_struct *tsec = current_security();
>> @@ -94,7 +95,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx
>> **ctxp, if (str_len >= PAGE_SIZE)
>>  		return -ENOMEM;
>>
>> -	ctx = kmalloc(sizeof(*ctx) + str_len + 1, GFP_KERNEL);
>> +	ctx = kmalloc(sizeof(*ctx) + str_len + 1, gfp);
>>  	if (!ctx)
>>  		return -ENOMEM;
> 
> Also located in selinux_xfrm_alloc_user() is a call to 
> security_context_to_sid() which calls security_context_to_sid_core() which in 
> some cases does allocate memory.  The good news is that to_sid_core() does 
> accept a gfp_t flag, the bad news is that to_sid() always passes GFP_KERNEL.
> 
> It looks like we need to extend this patch a bit, or add another.  Sorry about 
> that.  If you're getting tired of playing with the LSM/SELinux code let me 
> know :)
> 
Ah, right. I didn't follow all the paths, I'll fix up this patch and submit
a v2.
Thanks for the review,
 Nik

>> @@ -282,9 +283,10 @@ int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid)
>> * LSM hook implementation that allocs and transfers uctx spec to
>> xfrm_policy. */
>>  int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
>> -			      struct xfrm_user_sec_ctx *uctx)
>> +			      struct xfrm_user_sec_ctx *uctx,
>> +			      gfp_t gfp)
>>  {
>> -	return selinux_xfrm_alloc_user(ctxp, uctx);
>> +	return selinux_xfrm_alloc_user(ctxp, uctx, gfp);
>>  }
>>
>>  /*
>> @@ -332,7 +334,7 @@ int selinux_xfrm_policy_delete(struct xfrm_sec_ctx *ctx)
>> int selinux_xfrm_state_alloc(struct xfrm_state *x,
>>  			     struct xfrm_user_sec_ctx *uctx)
>>  {
>> -	return selinux_xfrm_alloc_user(&x->security, uctx);
>> +	return selinux_xfrm_alloc_user(&x->security, uctx, GFP_KERNEL);
>>  }
>>
>>  /*
> 

Re: [PATCH 0/2] af_key: fixes for sleeping while atomic

[Steffen Klassert]

On Tue, Mar 04, 2014 at 01:26:22PM +0100, Nikolay Aleksandrov wrote:
> Hi,
> The first patch takes care of the issue on the af_key side, and the second
> one fixes it on the selinux side (security_xfrm_policy_alloc).
> There're a few lines >80 but they were already long, I'm not sure if breaking
> them above the limit would make them more readable than they already are.
> 
> Best regards,
>  Nikolay Aleksandrov
> 
> CC: Paul Moore <paul@paul-moore.com>
> CC: Dave Jones <davej@redhat.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Fan Du <fan.du@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> 
> Nikolay Aleksandrov (2):
>   net: af_key: fix sleeping under rcu
>   selinux: add gfp argument to security_xfrm_policy_alloc and fix
>     callers
> 

Looks ok. I'd take both patches into the ipsec tree, but I want to see
an ack from Paul because this touches IPsec and selinux code.

Re: [PATCH 0/2] af_key: fixes for sleeping while atomic

[Paul Moore]

On Tuesday, March 04, 2014 01:26:22 PM Nikolay Aleksandrov wrote:
> Hi,
> The first patch takes care of the issue on the af_key side, and the second
> one fixes it on the selinux side (security_xfrm_policy_alloc).
> There're a few lines >80 but they were already long, I'm not sure if
> breaking them above the limit would make them more readable than they
> already are.
> 
> Best regards,
>  Nikolay Aleksandrov
> 
> CC: Paul Moore <paul@paul-moore.com>
> CC: Dave Jones <davej@redhat.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Fan Du <fan.du@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> 
> Nikolay Aleksandrov (2):
>   net: af_key: fix sleeping under rcu
>   selinux: add gfp argument to security_xfrm_policy_alloc and fix
>     callers

My apologies, I had hoped to review this today but other bugs got the better 
of me.  This is top of my list for tomorrow.

Thank you for putting this patchset together.

-- 
paul moore
www.paul-moore.com

[PATCHv2 0/2] af_key: fixes for sleeping while atomic

[Nikolay Aleksandrov]

Hi,
The first patch takes care of the issue on the af_key side, and the second
one fixes it on the selinux side (security_xfrm_policy_alloc).
There're a few lines >80 but they were already long, I'm not sure if breaking
them above the limit would make them more readable than they already are.
The v2 is because Paul caught a place which could do GFP_KERNEL allocation
from xfrm_policy_alloc_user, so I've adjusted that too.

Best regards,
 Nikolay Aleksandrov

CC: Paul Moore <paul@paul-moore.com>
CC: Dave Jones <davej@redhat.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Fan Du <fan.du@windriver.com>
CC: David S. Miller <davem@davemloft.net>

Nikolay Aleksandrov (2):
  net: af_key: fix sleeping under rcu
  selinux: add gfp argument to security_xfrm_policy_alloc and fix
    callers

 include/linux/security.h            | 10 +++++++---
 net/key/af_key.c                    | 19 ++++++++++---------
 net/xfrm/xfrm_user.c                |  6 +++---
 security/capability.c               |  3 ++-
 security/security.c                 |  6 ++++--
 security/selinux/hooks.c            | 13 +++++++------
 security/selinux/include/security.h |  2 +-
 security/selinux/include/xfrm.h     |  3 ++-
 security/selinux/selinuxfs.c        | 28 ++++++++++++++++++----------
 security/selinux/ss/services.c      |  6 ++++--
 security/selinux/xfrm.c             | 14 ++++++++------
 11 files changed, 66 insertions(+), 44 deletions(-)

-- 
1.8.5.3

[PATCHv2 1/2] net: af_key: fix sleeping under rcu

[Nikolay Aleksandrov]

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.

CC: Dave Jones <davej@redhat.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Fan Du <fan.du@windriver.com>
CC: David S. Miller <davem@davemloft.net>

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: No change

 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);
 		kfree(uctx);
 
-- 
1.8.5.3

[PATCHv2 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers

[Nikolay Aleksandrov]

security_xfrm_policy_alloc can be called in atomic context so the
allocation should be done with GFP_ATOMIC. Add an argument to let the
callers choose the appropriate way. In order to do so a gfp argument
needs to be added to the method xfrm_policy_alloc_security in struct
security_operations and to the internal function
selinux_xfrm_alloc_user. After that switch to GFP_ATOMIC in the atomic
callers and leave GFP_KERNEL as before for the rest.
The path that needed the gfp argument addition is:
security_xfrm_policy_alloc -> security_ops.xfrm_policy_alloc_security ->
all users of xfrm_policy_alloc_security (e.g. selinux_xfrm_policy_alloc) ->
selinux_xfrm_alloc_user (here the allocation used to be GFP_KERNEL only)

Now adding a gfp argument to selinux_xfrm_alloc_user requires us to also
add it to security_context_to_sid which is used inside and prior to this
patch did only GFP_KERNEL allocation. So add gfp argument to
security_context_to_sid and adjust all of its callers as well.

CC: Paul Moore <paul@paul-moore.com>
CC: Dave Jones <davej@redhat.com>
CC: Steffen Klassert <steffen.klassert@secunet.com>
CC: Fan Du <fan.du@windriver.com>
CC: David S. Miller <davem@davemloft.net>
CC: LSM list <linux-security-module@vger.kernel.org>
CC: SELinux list <selinux@tycho.nsa.gov>

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
v2: Add SELinux list to CCs, add gfp argument to security_context_to_sid
and adjust callers

 include/linux/security.h            | 10 +++++++---
 net/key/af_key.c                    |  6 +++---
 net/xfrm/xfrm_user.c                |  6 +++---
 security/capability.c               |  3 ++-
 security/security.c                 |  6 ++++--
 security/selinux/hooks.c            | 13 +++++++------
 security/selinux/include/security.h |  2 +-
 security/selinux/include/xfrm.h     |  3 ++-
 security/selinux/selinuxfs.c        | 28 ++++++++++++++++++----------
 security/selinux/ss/services.c      |  6 ++++--
 security/selinux/xfrm.c             | 14 ++++++++------
 11 files changed, 59 insertions(+), 38 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 5623a7f965b7..2fc42d191f79 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1040,6 +1040,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	Allocate a security structure to the xp->security field; the security
  *	field is initialized to NULL when the xfrm_policy is allocated.
  *	Return 0 if operation was successful (memory to allocate, legal context)
+ *	@gfp is to specify the context for the allocation
  * @xfrm_policy_clone_security:
  *	@old_ctx contains an existing xfrm_sec_ctx.
  *	@new_ctxp contains a new xfrm_sec_ctx being cloned from old.
@@ -1683,7 +1684,7 @@ struct security_operations {
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 	int (*xfrm_policy_alloc_security) (struct xfrm_sec_ctx **ctxp,
-			struct xfrm_user_sec_ctx *sec_ctx);
+			struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp);
 	int (*xfrm_policy_clone_security) (struct xfrm_sec_ctx *old_ctx, struct xfrm_sec_ctx **new_ctx);
 	void (*xfrm_policy_free_security) (struct xfrm_sec_ctx *ctx);
 	int (*xfrm_policy_delete_security) (struct xfrm_sec_ctx *ctx);
@@ -2859,7 +2860,8 @@ static inline void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 
-int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct xfrm_user_sec_ctx *sec_ctx);
+int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
+			       struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp);
 int security_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx, struct xfrm_sec_ctx **new_ctxp);
 void security_xfrm_policy_free(struct xfrm_sec_ctx *ctx);
 int security_xfrm_policy_delete(struct xfrm_sec_ctx *ctx);
@@ -2877,7 +2879,9 @@ void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl);
 
 #else	/* CONFIG_SECURITY_NETWORK_XFRM */
 
-static inline int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct xfrm_user_sec_ctx *sec_ctx)
+static inline int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
+					     struct xfrm_user_sec_ctx *sec_ctx,
+					     gfp_t gfp)
 {
 	return 0;
 }
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 1526023f99ed..79326978517a 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2239,7 +2239,7 @@ static int pfkey_spdadd(struct sock *sk, struct sk_buff *skb, const struct sadb_
 			goto out;
 		}
 
-		err = security_xfrm_policy_alloc(&xp->security, uctx);
+		err = security_xfrm_policy_alloc(&xp->security, uctx, GFP_KERNEL);
 		kfree(uctx);
 
 		if (err)
@@ -2341,7 +2341,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
 		if (!uctx)
 			return -ENOMEM;
 
-		err = security_xfrm_policy_alloc(&pol_ctx, uctx);
+		err = security_xfrm_policy_alloc(&pol_ctx, uctx, GFP_KERNEL);
 		kfree(uctx);
 		if (err)
 			return err;
@@ -3241,7 +3241,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, GFP_ATOMIC);
-		*dir = security_xfrm_policy_alloc(&xp->security, uctx);
+		*dir = security_xfrm_policy_alloc(&xp->security, uctx, GFP_ATOMIC);
 		kfree(uctx);
 
 		if (*dir)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index c274179d60a2..2f7ddc3a59b4 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1221,7 +1221,7 @@ static int copy_from_user_sec_ctx(struct xfrm_policy *pol, struct nlattr **attrs
 		return 0;
 
 	uctx = nla_data(rt);
-	return security_xfrm_policy_alloc(&pol->security, uctx);
+	return security_xfrm_policy_alloc(&pol->security, uctx, GFP_KERNEL);
 }
 
 static void copy_templates(struct xfrm_policy *xp, struct xfrm_user_tmpl *ut,
@@ -1626,7 +1626,7 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 		if (rt) {
 			struct xfrm_user_sec_ctx *uctx = nla_data(rt);
 
-			err = security_xfrm_policy_alloc(&ctx, uctx);
+			err = security_xfrm_policy_alloc(&ctx, uctx, GFP_KERNEL);
 			if (err)
 				return err;
 		}
@@ -1928,7 +1928,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 		if (rt) {
 			struct xfrm_user_sec_ctx *uctx = nla_data(rt);
 
-			err = security_xfrm_policy_alloc(&ctx, uctx);
+			err = security_xfrm_policy_alloc(&ctx, uctx, GFP_KERNEL);
 			if (err)
 				return err;
 		}
diff --git a/security/capability.c b/security/capability.c
index 8b4f24ae4338..21e2b9cae685 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -757,7 +757,8 @@ static void cap_skb_owned_by(struct sk_buff *skb, struct sock *sk)
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 static int cap_xfrm_policy_alloc_security(struct xfrm_sec_ctx **ctxp,
-					  struct xfrm_user_sec_ctx *sec_ctx)
+					  struct xfrm_user_sec_ctx *sec_ctx,
+					  gfp_t gfp)
 {
 	return 0;
 }
diff --git a/security/security.c b/security/security.c
index 15b6928592ef..919cad93ac82 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1317,9 +1317,11 @@ void security_skb_owned_by(struct sk_buff *skb, struct sock *sk)
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
 
-int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct xfrm_user_sec_ctx *sec_ctx)
+int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
+			       struct xfrm_user_sec_ctx *sec_ctx,
+			       gfp_t gfp)
 {
-	return security_ops->xfrm_policy_alloc_security(ctxp, sec_ctx);
+	return security_ops->xfrm_policy_alloc_security(ctxp, sec_ctx, gfp);
 }
 EXPORT_SYMBOL(security_xfrm_policy_alloc);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4b34847208cc..b332e2cc0954 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -668,7 +668,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 		if (flags[i] == SBLABEL_MNT)
 			continue;
 		rc = security_context_to_sid(mount_options[i],
-					     strlen(mount_options[i]), &sid);
+					     strlen(mount_options[i]), &sid, GFP_KERNEL);
 		if (rc) {
 			printk(KERN_WARNING "SELinux: security_context_to_sid"
 			       "(%s) failed for (dev %s, type %s) errno=%d\n",
@@ -2489,7 +2489,8 @@ static int selinux_sb_remount(struct super_block *sb, void *data)
 		if (flags[i] == SBLABEL_MNT)
 			continue;
 		len = strlen(mount_options[i]);
-		rc = security_context_to_sid(mount_options[i], len, &sid);
+		rc = security_context_to_sid(mount_options[i], len, &sid,
+					     GFP_KERNEL);
 		if (rc) {
 			printk(KERN_WARNING "SELinux: security_context_to_sid"
 			       "(%s) failed for (dev %s, type %s) errno=%d\n",
@@ -2893,7 +2894,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
 	if (rc)
 		return rc;
 
-	rc = security_context_to_sid(value, size, &newsid);
+	rc = security_context_to_sid(value, size, &newsid, GFP_KERNEL);
 	if (rc == -EINVAL) {
 		if (!capable(CAP_MAC_ADMIN)) {
 			struct audit_buffer *ab;
@@ -3050,7 +3051,7 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
 	if (!value || !size)
 		return -EACCES;
 
-	rc = security_context_to_sid((void *)value, size, &newsid);
+	rc = security_context_to_sid((void *)value, size, &newsid, GFP_KERNEL);
 	if (rc)
 		return rc;
 
@@ -5529,7 +5530,7 @@ static int selinux_setprocattr(struct task_struct *p,
 			str[size-1] = 0;
 			size--;
 		}
-		error = security_context_to_sid(value, size, &sid);
+		error = security_context_to_sid(value, size, &sid, GFP_KERNEL);
 		if (error == -EINVAL && !strcmp(name, "fscreate")) {
 			if (!capable(CAP_MAC_ADMIN)) {
 				struct audit_buffer *ab;
@@ -5638,7 +5639,7 @@ static int selinux_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 
 static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
 {
-	return security_context_to_sid(secdata, seclen, secid);
+	return security_context_to_sid(secdata, seclen, secid, GFP_KERNEL);
 }
 
 static void selinux_release_secctx(char *secdata, u32 seclen)
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 8ed8daf7f1ee..ce7852cf526b 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -134,7 +134,7 @@ int security_sid_to_context(u32 sid, char **scontext,
 int security_sid_to_context_force(u32 sid, char **scontext, u32 *scontext_len);
 
 int security_context_to_sid(const char *scontext, u32 scontext_len,
-	u32 *out_sid);
+			    u32 *out_sid, gfp_t gfp);
 
 int security_context_to_sid_default(const char *scontext, u32 scontext_len,
 				    u32 *out_sid, u32 def_sid, gfp_t gfp_flags);
diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h
index 48c3cc94c168..9f0584710c85 100644
--- a/security/selinux/include/xfrm.h
+++ b/security/selinux/include/xfrm.h
@@ -10,7 +10,8 @@
 #include <net/flow.h>
 
 int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
-			      struct xfrm_user_sec_ctx *uctx);
+			      struct xfrm_user_sec_ctx *uctx,
+			      gfp_t gfp);
 int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
 			      struct xfrm_sec_ctx **new_ctxp);
 void selinux_xfrm_policy_free(struct xfrm_sec_ctx *ctx);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 5122affe06a8..d60c0ee66387 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -576,7 +576,7 @@ static ssize_t sel_write_context(struct file *file, char *buf, size_t size)
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(buf, size, &sid);
+	length = security_context_to_sid(buf, size, &sid, GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -731,11 +731,13 @@ static ssize_t sel_write_access(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
 		goto out;
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
+	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
+	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -817,11 +819,13 @@ static ssize_t sel_write_create(struct file *file, char *buf, size_t size)
 		objname = namebuf;
 	}
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
+	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
+	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -878,11 +882,13 @@ static ssize_t sel_write_relabel(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
 		goto out;
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
+	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
+	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -934,7 +940,7 @@ static ssize_t sel_write_user(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s", con, user) != 2)
 		goto out;
 
-	length = security_context_to_sid(con, strlen(con) + 1, &sid);
+	length = security_context_to_sid(con, strlen(con) + 1, &sid, GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -994,11 +1000,13 @@ static ssize_t sel_write_member(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
 		goto out;
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
+	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
+	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
+					 GFP_KERNEL);
 	if (length)
 		goto out;
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 5d0144ee8ed6..4bca49414a40 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1289,16 +1289,18 @@ out:
  * @scontext: security context
  * @scontext_len: length in bytes
  * @sid: security identifier, SID
+ * @gfp: context for the allocation
  *
  * Obtains a SID associated with the security context that
  * has the string representation specified by @scontext.
  * Returns -%EINVAL if the context is invalid, -%ENOMEM if insufficient
  * memory is available, or 0 on success.
  */
-int security_context_to_sid(const char *scontext, u32 scontext_len, u32 *sid)
+int security_context_to_sid(const char *scontext, u32 scontext_len, u32 *sid,
+			    gfp_t gfp)
 {
 	return security_context_to_sid_core(scontext, scontext_len,
-					    sid, SECSID_NULL, GFP_KERNEL, 0);
+					    sid, SECSID_NULL, gfp, 0);
 }
 
 /**
diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
index 0462cb3ff0a7..98b042630a9e 100644
--- a/security/selinux/xfrm.c
+++ b/security/selinux/xfrm.c
@@ -78,7 +78,8 @@ static inline int selinux_authorizable_xfrm(struct xfrm_state *x)
  * xfrm_user_sec_ctx context.
  */
 static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
-				   struct xfrm_user_sec_ctx *uctx)
+				   struct xfrm_user_sec_ctx *uctx,
+				   gfp_t gfp)
 {
 	int rc;
 	const struct task_security_struct *tsec = current_security();
@@ -94,7 +95,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
 	if (str_len >= PAGE_SIZE)
 		return -ENOMEM;
 
-	ctx = kmalloc(sizeof(*ctx) + str_len + 1, GFP_KERNEL);
+	ctx = kmalloc(sizeof(*ctx) + str_len + 1, gfp);
 	if (!ctx)
 		return -ENOMEM;
 
@@ -103,7 +104,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
 	ctx->ctx_len = str_len;
 	memcpy(ctx->ctx_str, &uctx[1], str_len);
 	ctx->ctx_str[str_len] = '\0';
-	rc = security_context_to_sid(ctx->ctx_str, str_len, &ctx->ctx_sid);
+	rc = security_context_to_sid(ctx->ctx_str, str_len, &ctx->ctx_sid, gfp);
 	if (rc)
 		goto err;
 
@@ -282,9 +283,10 @@ int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid)
  * LSM hook implementation that allocs and transfers uctx spec to xfrm_policy.
  */
 int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
-			      struct xfrm_user_sec_ctx *uctx)
+			      struct xfrm_user_sec_ctx *uctx,
+			      gfp_t gfp)
 {
-	return selinux_xfrm_alloc_user(ctxp, uctx);
+	return selinux_xfrm_alloc_user(ctxp, uctx, gfp);
 }
 
 /*
@@ -332,7 +334,7 @@ int selinux_xfrm_policy_delete(struct xfrm_sec_ctx *ctx)
 int selinux_xfrm_state_alloc(struct xfrm_state *x,
 			     struct xfrm_user_sec_ctx *uctx)
 {
-	return selinux_xfrm_alloc_user(&x->security, uctx);
+	return selinux_xfrm_alloc_user(&x->security, uctx, GFP_KERNEL);
 }
 
 /*
-- 
1.8.5.3

Re: [PATCHv2 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers

[Paul Moore]

On Friday, March 07, 2014 12:44:19 PM Nikolay Aleksandrov wrote:
> security_xfrm_policy_alloc can be called in atomic context so the
> allocation should be done with GFP_ATOMIC. Add an argument to let the
> callers choose the appropriate way. In order to do so a gfp argument
> needs to be added to the method xfrm_policy_alloc_security in struct
> security_operations and to the internal function
> selinux_xfrm_alloc_user. After that switch to GFP_ATOMIC in the atomic
> callers and leave GFP_KERNEL as before for the rest.
> The path that needed the gfp argument addition is:
> security_xfrm_policy_alloc -> security_ops.xfrm_policy_alloc_security ->
> all users of xfrm_policy_alloc_security (e.g. selinux_xfrm_policy_alloc) ->
> selinux_xfrm_alloc_user (here the allocation used to be GFP_KERNEL only)
> 
> Now adding a gfp argument to selinux_xfrm_alloc_user requires us to also
> add it to security_context_to_sid which is used inside and prior to this
> patch did only GFP_KERNEL allocation. So add gfp argument to
> security_context_to_sid and adjust all of its callers as well.
> 
> CC: Paul Moore <paul@paul-moore.com>
> CC: Dave Jones <davej@redhat.com>
> CC: Steffen Klassert <steffen.klassert@secunet.com>
> CC: Fan Du <fan.du@windriver.com>
> CC: David S. Miller <davem@davemloft.net>
> CC: LSM list <linux-security-module@vger.kernel.org>
> CC: SELinux list <selinux@tycho.nsa.gov>
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>

This looks good to me, thanks for finding this and following through with a 
patch.

Acked-by: Paul Moore <paul@paul-moore.com>

> ---
> v2: Add SELinux list to CCs, add gfp argument to security_context_to_sid
> and adjust callers
> 
>  include/linux/security.h            | 10 +++++++---
>  net/key/af_key.c                    |  6 +++---
>  net/xfrm/xfrm_user.c                |  6 +++---
>  security/capability.c               |  3 ++-
>  security/security.c                 |  6 ++++--
>  security/selinux/hooks.c            | 13 +++++++------
>  security/selinux/include/security.h |  2 +-
>  security/selinux/include/xfrm.h     |  3 ++-
>  security/selinux/selinuxfs.c        | 28 ++++++++++++++++++----------
>  security/selinux/ss/services.c      |  6 ++++--
>  security/selinux/xfrm.c             | 14 ++++++++------
>  11 files changed, 59 insertions(+), 38 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 5623a7f965b7..2fc42d191f79 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1040,6 +1040,7 @@ static inline void security_free_mnt_opts(struct
> security_mnt_opts *opts) *	Allocate a security structure to the
> xp->security field; the security *	field is initialized to NULL when the
> xfrm_policy is allocated. *	Return 0 if operation was successful (memory 
to
> allocate, legal context) + *	@gfp is to specify the context for the
> allocation
>   * @xfrm_policy_clone_security:
>   *	@old_ctx contains an existing xfrm_sec_ctx.
>   *	@new_ctxp contains a new xfrm_sec_ctx being cloned from old.
> @@ -1683,7 +1684,7 @@ struct security_operations {
> 
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  	int (*xfrm_policy_alloc_security) (struct xfrm_sec_ctx **ctxp,
> -			struct xfrm_user_sec_ctx *sec_ctx);
> +			struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp);
>  	int (*xfrm_policy_clone_security) (struct xfrm_sec_ctx *old_ctx, struct
> xfrm_sec_ctx **new_ctx); void (*xfrm_policy_free_security) (struct
> xfrm_sec_ctx *ctx);
>  	int (*xfrm_policy_delete_security) (struct xfrm_sec_ctx *ctx);
> @@ -2859,7 +2860,8 @@ static inline void security_skb_owned_by(struct
> sk_buff *skb, struct sock *sk)
> 
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> 
> -int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct
> xfrm_user_sec_ctx *sec_ctx); +int security_xfrm_policy_alloc(struct
> xfrm_sec_ctx **ctxp,
> +			       struct xfrm_user_sec_ctx *sec_ctx, gfp_t gfp);
>  int security_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx, struct
> xfrm_sec_ctx **new_ctxp); void security_xfrm_policy_free(struct
> xfrm_sec_ctx *ctx);
>  int security_xfrm_policy_delete(struct xfrm_sec_ctx *ctx);
> @@ -2877,7 +2879,9 @@ void security_skb_classify_flow(struct sk_buff *skb,
> struct flowi *fl);
> 
>  #else	/* CONFIG_SECURITY_NETWORK_XFRM */
> 
> -static inline int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
> struct xfrm_user_sec_ctx *sec_ctx) +static inline int
> security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, +					
     struct
> xfrm_user_sec_ctx *sec_ctx,
> +					     gfp_t gfp)
>  {
>  	return 0;
>  }
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 1526023f99ed..79326978517a 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -2239,7 +2239,7 @@ static int pfkey_spdadd(struct sock *sk, struct
> sk_buff *skb, const struct sadb_ goto out;
>  		}
> 
> -		err = security_xfrm_policy_alloc(&xp->security, uctx);
> +		err = security_xfrm_policy_alloc(&xp->security, uctx, GFP_KERNEL);
>  		kfree(uctx);
> 
>  		if (err)
> @@ -2341,7 +2341,7 @@ static int pfkey_spddelete(struct sock *sk, struct
> sk_buff *skb, const struct sa if (!uctx)
>  			return -ENOMEM;
> 
> -		err = security_xfrm_policy_alloc(&pol_ctx, uctx);
> +		err = security_xfrm_policy_alloc(&pol_ctx, uctx, GFP_KERNEL);
>  		kfree(uctx);
>  		if (err)
>  			return err;
> @@ -3241,7 +3241,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, GFP_ATOMIC);
> -		*dir = security_xfrm_policy_alloc(&xp->security, uctx);
> +		*dir = security_xfrm_policy_alloc(&xp->security, uctx, GFP_ATOMIC);
>  		kfree(uctx);
> 
>  		if (*dir)
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index c274179d60a2..2f7ddc3a59b4 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1221,7 +1221,7 @@ static int copy_from_user_sec_ctx(struct xfrm_policy
> *pol, struct nlattr **attrs return 0;
> 
>  	uctx = nla_data(rt);
> -	return security_xfrm_policy_alloc(&pol->security, uctx);
> +	return security_xfrm_policy_alloc(&pol->security, uctx, GFP_KERNEL);
>  }
> 
>  static void copy_templates(struct xfrm_policy *xp, struct xfrm_user_tmpl
> *ut, @@ -1626,7 +1626,7 @@ static int xfrm_get_policy(struct sk_buff *skb,
> struct nlmsghdr *nlh, if (rt) {
>  			struct xfrm_user_sec_ctx *uctx = nla_data(rt);
> 
> -			err = security_xfrm_policy_alloc(&ctx, uctx);
> +			err = security_xfrm_policy_alloc(&ctx, uctx, GFP_KERNEL);
>  			if (err)
>  				return err;
>  		}
> @@ -1928,7 +1928,7 @@ static int xfrm_add_pol_expire(struct sk_buff *skb,
> struct nlmsghdr *nlh, if (rt) {
>  			struct xfrm_user_sec_ctx *uctx = nla_data(rt);
> 
> -			err = security_xfrm_policy_alloc(&ctx, uctx);
> +			err = security_xfrm_policy_alloc(&ctx, uctx, GFP_KERNEL);
>  			if (err)
>  				return err;
>  		}
> diff --git a/security/capability.c b/security/capability.c
> index 8b4f24ae4338..21e2b9cae685 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -757,7 +757,8 @@ static void cap_skb_owned_by(struct sk_buff *skb, struct
> sock *sk)
> 
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  static int cap_xfrm_policy_alloc_security(struct xfrm_sec_ctx **ctxp,
> -					  struct xfrm_user_sec_ctx *sec_ctx)
> +					  struct xfrm_user_sec_ctx *sec_ctx,
> +					  gfp_t gfp)
>  {
>  	return 0;
>  }
> diff --git a/security/security.c b/security/security.c
> index 15b6928592ef..919cad93ac82 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1317,9 +1317,11 @@ void security_skb_owned_by(struct sk_buff *skb,
> struct sock *sk)
> 
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> 
> -int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp, struct
> xfrm_user_sec_ctx *sec_ctx) +int security_xfrm_policy_alloc(struct
> xfrm_sec_ctx **ctxp,
> +			       struct xfrm_user_sec_ctx *sec_ctx,
> +			       gfp_t gfp)
>  {
> -	return security_ops->xfrm_policy_alloc_security(ctxp, sec_ctx);
> +	return security_ops->xfrm_policy_alloc_security(ctxp, sec_ctx, gfp);
>  }
>  EXPORT_SYMBOL(security_xfrm_policy_alloc);
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 4b34847208cc..b332e2cc0954 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -668,7 +668,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>  		if (flags[i] == SBLABEL_MNT)
>  			continue;
>  		rc = security_context_to_sid(mount_options[i],
> -					     strlen(mount_options[i]), &sid);
> +					     strlen(mount_options[i]), &sid, GFP_KERNEL);
>  		if (rc) {
>  			printk(KERN_WARNING "SELinux: security_context_to_sid"
>  			       "(%s) failed for (dev %s, type %s) errno=%d\n",
> @@ -2489,7 +2489,8 @@ static int selinux_sb_remount(struct super_block *sb,
> void *data) if (flags[i] == SBLABEL_MNT)
>  			continue;
>  		len = strlen(mount_options[i]);
> -		rc = security_context_to_sid(mount_options[i], len, &sid);
> +		rc = security_context_to_sid(mount_options[i], len, &sid,
> +					     GFP_KERNEL);
>  		if (rc) {
>  			printk(KERN_WARNING "SELinux: security_context_to_sid"
>  			       "(%s) failed for (dev %s, type %s) errno=%d\n",
> @@ -2893,7 +2894,7 @@ static int selinux_inode_setxattr(struct dentry
> *dentry, const char *name, if (rc)
>  		return rc;
> 
> -	rc = security_context_to_sid(value, size, &newsid);
> +	rc = security_context_to_sid(value, size, &newsid, GFP_KERNEL);
>  	if (rc == -EINVAL) {
>  		if (!capable(CAP_MAC_ADMIN)) {
>  			struct audit_buffer *ab;
> @@ -3050,7 +3051,7 @@ static int selinux_inode_setsecurity(struct inode
> *inode, const char *name, if (!value || !size)
>  		return -EACCES;
> 
> -	rc = security_context_to_sid((void *)value, size, &newsid);
> +	rc = security_context_to_sid((void *)value, size, &newsid, GFP_KERNEL);
>  	if (rc)
>  		return rc;
> 
> @@ -5529,7 +5530,7 @@ static int selinux_setprocattr(struct task_struct *p,
>  			str[size-1] = 0;
>  			size--;
>  		}
> -		error = security_context_to_sid(value, size, &sid);
> +		error = security_context_to_sid(value, size, &sid, GFP_KERNEL);
>  		if (error == -EINVAL && !strcmp(name, "fscreate")) {
>  			if (!capable(CAP_MAC_ADMIN)) {
>  				struct audit_buffer *ab;
> @@ -5638,7 +5639,7 @@ static int selinux_secid_to_secctx(u32 secid, char
> **secdata, u32 *seclen)
> 
>  static int selinux_secctx_to_secid(const char *secdata, u32 seclen, u32
> *secid) {
> -	return security_context_to_sid(secdata, seclen, secid);
> +	return security_context_to_sid(secdata, seclen, secid, GFP_KERNEL);
>  }
> 
>  static void selinux_release_secctx(char *secdata, u32 seclen)
> diff --git a/security/selinux/include/security.h
> b/security/selinux/include/security.h index 8ed8daf7f1ee..ce7852cf526b
> 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -134,7 +134,7 @@ int security_sid_to_context(u32 sid, char **scontext,
>  int security_sid_to_context_force(u32 sid, char **scontext, u32
> *scontext_len);
> 
>  int security_context_to_sid(const char *scontext, u32 scontext_len,
> -	u32 *out_sid);
> +			    u32 *out_sid, gfp_t gfp);
> 
>  int security_context_to_sid_default(const char *scontext, u32 scontext_len,
> u32 *out_sid, u32 def_sid, gfp_t gfp_flags);
> diff --git a/security/selinux/include/xfrm.h
> b/security/selinux/include/xfrm.h index 48c3cc94c168..9f0584710c85 100644
> --- a/security/selinux/include/xfrm.h
> +++ b/security/selinux/include/xfrm.h
> @@ -10,7 +10,8 @@
>  #include <net/flow.h>
> 
>  int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
> -			      struct xfrm_user_sec_ctx *uctx);
> +			      struct xfrm_user_sec_ctx *uctx,
> +			      gfp_t gfp);
>  int selinux_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
>  			      struct xfrm_sec_ctx **new_ctxp);
>  void selinux_xfrm_policy_free(struct xfrm_sec_ctx *ctx);
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 5122affe06a8..d60c0ee66387 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -576,7 +576,7 @@ static ssize_t sel_write_context(struct file *file, char
> *buf, size_t size) if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(buf, size, &sid);
> +	length = security_context_to_sid(buf, size, &sid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -731,11 +731,13 @@ static ssize_t sel_write_access(struct file *file,
> char *buf, size_t size) if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass)
> != 3)
>  		goto out;
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
> +	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
> +	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -817,11 +819,13 @@ static ssize_t sel_write_create(struct file *file,
> char *buf, size_t size) objname = namebuf;
>  	}
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
> +	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
> +	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -878,11 +882,13 @@ static ssize_t sel_write_relabel(struct file *file,
> char *buf, size_t size) if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass)
> != 3)
>  		goto out;
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
> +	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
> +	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -934,7 +940,7 @@ static ssize_t sel_write_user(struct file *file, char
> *buf, size_t size) if (sscanf(buf, "%s %s", con, user) != 2)
>  		goto out;
> 
> -	length = security_context_to_sid(con, strlen(con) + 1, &sid);
> +	length = security_context_to_sid(con, strlen(con) + 1, &sid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -994,11 +1000,13 @@ static ssize_t sel_write_member(struct file *file,
> char *buf, size_t size) if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass)
> != 3)
>  		goto out;
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid);
> +	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid);
> +	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> +					 GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 5d0144ee8ed6..4bca49414a40 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1289,16 +1289,18 @@ out:
>   * @scontext: security context
>   * @scontext_len: length in bytes
>   * @sid: security identifier, SID
> + * @gfp: context for the allocation
>   *
>   * Obtains a SID associated with the security context that
>   * has the string representation specified by @scontext.
>   * Returns -%EINVAL if the context is invalid, -%ENOMEM if insufficient
>   * memory is available, or 0 on success.
>   */
> -int security_context_to_sid(const char *scontext, u32 scontext_len, u32
> *sid) +int security_context_to_sid(const char *scontext, u32 scontext_len,
> u32 *sid, +			    gfp_t gfp)
>  {
>  	return security_context_to_sid_core(scontext, scontext_len,
> -					    sid, SECSID_NULL, GFP_KERNEL, 0);
> +					    sid, SECSID_NULL, gfp, 0);
>  }
> 
>  /**
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 0462cb3ff0a7..98b042630a9e 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -78,7 +78,8 @@ static inline int selinux_authorizable_xfrm(struct
> xfrm_state *x) * xfrm_user_sec_ctx context.
>   */
>  static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx **ctxp,
> -				   struct xfrm_user_sec_ctx *uctx)
> +				   struct xfrm_user_sec_ctx *uctx,
> +				   gfp_t gfp)
>  {
>  	int rc;
>  	const struct task_security_struct *tsec = current_security();
> @@ -94,7 +95,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx
> **ctxp, if (str_len >= PAGE_SIZE)
>  		return -ENOMEM;
> 
> -	ctx = kmalloc(sizeof(*ctx) + str_len + 1, GFP_KERNEL);
> +	ctx = kmalloc(sizeof(*ctx) + str_len + 1, gfp);
>  	if (!ctx)
>  		return -ENOMEM;
> 
> @@ -103,7 +104,7 @@ static int selinux_xfrm_alloc_user(struct xfrm_sec_ctx
> **ctxp, ctx->ctx_len = str_len;
>  	memcpy(ctx->ctx_str, &uctx[1], str_len);
>  	ctx->ctx_str[str_len] = '\0';
> -	rc = security_context_to_sid(ctx->ctx_str, str_len, &ctx->ctx_sid);
> +	rc = security_context_to_sid(ctx->ctx_str, str_len, &ctx->ctx_sid, gfp);
>  	if (rc)
>  		goto err;
> 
> @@ -282,9 +283,10 @@ int selinux_xfrm_skb_sid(struct sk_buff *skb, u32 *sid)
> * LSM hook implementation that allocs and transfers uctx spec to
> xfrm_policy. */
>  int selinux_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
> -			      struct xfrm_user_sec_ctx *uctx)
> +			      struct xfrm_user_sec_ctx *uctx,
> +			      gfp_t gfp)
>  {
> -	return selinux_xfrm_alloc_user(ctxp, uctx);
> +	return selinux_xfrm_alloc_user(ctxp, uctx, gfp);
>  }
> 
>  /*
> @@ -332,7 +334,7 @@ int selinux_xfrm_policy_delete(struct xfrm_sec_ctx *ctx)
> int selinux_xfrm_state_alloc(struct xfrm_state *x,
>  			     struct xfrm_user_sec_ctx *uctx)
>  {
> -	return selinux_xfrm_alloc_user(&x->security, uctx);
> +	return selinux_xfrm_alloc_user(&x->security, uctx, GFP_KERNEL);
>  }
> 
>  /*

-- 
paul moore
www.paul-moore.com

Re: [PATCHv2 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers

[Steffen Klassert]

On Fri, Mar 07, 2014 at 05:27:17PM -0500, Paul Moore wrote:
> On Friday, March 07, 2014 12:44:19 PM Nikolay Aleksandrov wrote:
> > security_xfrm_policy_alloc can be called in atomic context so the
> > allocation should be done with GFP_ATOMIC. Add an argument to let the
> > callers choose the appropriate way. In order to do so a gfp argument
> > needs to be added to the method xfrm_policy_alloc_security in struct
> > security_operations and to the internal function
> > selinux_xfrm_alloc_user. After that switch to GFP_ATOMIC in the atomic
> > callers and leave GFP_KERNEL as before for the rest.
> > The path that needed the gfp argument addition is:
> > security_xfrm_policy_alloc -> security_ops.xfrm_policy_alloc_security ->
> > all users of xfrm_policy_alloc_security (e.g. selinux_xfrm_policy_alloc) ->
> > selinux_xfrm_alloc_user (here the allocation used to be GFP_KERNEL only)
> > 
> > Now adding a gfp argument to selinux_xfrm_alloc_user requires us to also
> > add it to security_context_to_sid which is used inside and prior to this
> > patch did only GFP_KERNEL allocation. So add gfp argument to
> > security_context_to_sid and adjust all of its callers as well.
> > 
> > CC: Paul Moore <paul@paul-moore.com>
> > CC: Dave Jones <davej@redhat.com>
> > CC: Steffen Klassert <steffen.klassert@secunet.com>
> > CC: Fan Du <fan.du@windriver.com>
> > CC: David S. Miller <davem@davemloft.net>
> > CC: LSM list <linux-security-module@vger.kernel.org>
> > CC: SELinux list <selinux@tycho.nsa.gov>
> > 
> > Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
> 
> This looks good to me, thanks for finding this and following through with a 
> patch.
> 
> Acked-by: Paul Moore <paul@paul-moore.com>
> 

Both patches applied to the ipsec tree.

Thanks everyone!