netdev - Re: [PATCH 2/2] selinux: add gfp argument to security_xfrm_policy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5319A4D3.1050501@redhat.com>
Date:	Fri, 07 Mar 2014 11:52:03 +0100
From:	Nikolay Aleksandrov <nikolay@...hat.com>
To:	Paul Moore <paul@...l-moore.com>
CC:	netdev@...r.kernel.org, Dave Jones <davej@...hat.com>,
	Steffen Klassert <steffen.klassert@...unet.com>,
	Fan Du <fan.du@...driver.com>,
	"David S. Miller" <davem@...emloft.net>,
	LSM list <linux-security-module@...r.kernel.org>,
	selinux@...ho.nsa.gov
Subject: Re: [PATCH 2/2] selinux: add gfp argument to security_xfrm_policy_alloc
 and fix callers

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@...l-moore.com>
>> CC: Dave Jones <davej@...hat.com>
>> CC: Steffen Klassert <steffen.klassert@...unet.com>
>> CC: Fan Du <fan.du@...driver.com>
>> CC: David S. Miller <davem@...emloft.net>
>> CC: LSM list <linux-security-module@...r.kernel.org>
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@...hat.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);
>>  }
>>
>>  /*
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html