From: Nikolay Aleksandrov <nikolay@redhat•com>
To: Steffen Klassert <steffen.klassert@secunet•com>
Cc: netdev@vger•kernel.org, Dave Jones <davej@redhat•com>,
Fan Du <fan.du@windriver•com>,
"David S. Miller" <davem@davemloft•net>,
Paul Moore <paul@paul-moore•com>,
linux-security-module@vger•kernel.org
Subject: Re: Possible fix
Date: Fri, 28 Feb 2014 11:10:07 +0100 [thread overview]
Message-ID: <5310607F.7030401@redhat.com> (raw)
In-Reply-To: <20140228072333.GP32371@secunet.com>
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
next prev parent reply other threads:[~2014-02-28 10:10 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-27 15:19 kmalloc with locks held in xfrm Dave Jones
2014-02-27 16:17 ` Possible fix Nikolay Aleksandrov
2014-02-27 16:24 ` Nikolay Aleksandrov
2014-02-27 17:05 ` Nikolay Aleksandrov
2014-02-28 7:23 ` Steffen Klassert
2014-02-28 10:10 ` Nikolay Aleksandrov [this message]
2014-02-28 22:10 ` Paul Moore
2014-03-02 16:26 ` Nikolay Aleksandrov
2014-03-05 12:20 ` Steffen Klassert
2014-03-07 3:04 ` Paul Moore
2014-03-07 11:23 ` Steffen Klassert
2014-03-07 15:50 ` Paul Moore
2014-03-04 12:26 ` [PATCH 0/2] af_key: fixes for sleeping while atomic Nikolay Aleksandrov
2014-03-04 12:26 ` [PATCH 1/2] net: af_key: fix sleeping under rcu Nikolay Aleksandrov
2014-03-04 12:46 ` David Laight
2014-03-04 21:40 ` David Miller
2014-03-04 12:26 ` [PATCH 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers Nikolay Aleksandrov
2014-03-07 3:22 ` Paul Moore
2014-03-07 10:52 ` Nikolay Aleksandrov
2014-03-05 12:07 ` [PATCH 0/2] af_key: fixes for sleeping while atomic Steffen Klassert
2014-03-05 22:21 ` Paul Moore
2014-03-07 11:44 ` [PATCHv2 " Nikolay Aleksandrov
2014-03-07 11:44 ` [PATCHv2 1/2] net: af_key: fix sleeping under rcu Nikolay Aleksandrov
2014-03-07 11:44 ` [PATCHv2 2/2] selinux: add gfp argument to security_xfrm_policy_alloc and fix callers Nikolay Aleksandrov
2014-03-07 22:27 ` Paul Moore
2014-03-10 12:52 ` Steffen Klassert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5310607F.7030401@redhat.com \
--to=nikolay@redhat$(echo .)com \
--cc=davej@redhat$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=fan.du@windriver$(echo .)com \
--cc=linux-security-module@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=paul@paul-moore$(echo .)com \
--cc=steffen.klassert@secunet$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox