netdev - Re: [PATCH net-next] {selinux, af_key} Rework pfkey_sadb2xfrm_user_sec

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 17 Oct 2013 09:34:53 +0800
From:	Fan Du <fan.du@...driver.com>
To:	Paul Moore <paul@...l-moore.com>
CC:	<steffen.klassert@...unet.com>, <davem@...emloft.net>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] {selinux, af_key} Rework pfkey_sadb2xfrm_user_sec_ctx



On 2013年10月16日 23:15, Paul Moore wrote:
> On Wednesday, October 16, 2013 02:15:14 PM Fan Du wrote:
>> Taking advantages of sadb_x_sec_ctx and xfrm_user_sec_ctx share the same
>> structure arrangement, rework pfkey_sadb2xfrm_user_sec_ctx by casting
>> sadb_x_sec_ctx into xfrm_user_sec_ctx with minor len fix.
>>
>> Then we can:
>>   -Avoid kmalloc/free memory for xfrm_user_sec_ctx, sadb_x_sec_ctx would be
>>    fine.
>>   -Fix missing return value check bug for pfkey_compile_policy when
>>    kmalloc fails
>>
>> Signed-off-by: Fan Du<fan.du@...driver.com>
>> ---
>>   net/key/af_key.c |   33 +--------------------------------
>>   1 file changed, 1 insertion(+), 32 deletions(-)
>>
>> diff --git a/net/key/af_key.c b/net/key/af_key.c
>> index 9d58537..c7d304d 100644
>> --- a/net/key/af_key.c
>> +++ b/net/key/af_key.c
>> @@ -435,22 +435,9 @@ static inline int verify_sec_ctx_len(const void *p)
>>
>>   static inline struct xfrm_user_sec_ctx *pfkey_sadb2xfrm_user_sec_ctx(const
>> struct sadb_x_sec_ctx *sec_ctx) {
>> -	struct xfrm_user_sec_ctx *uctx = NULL;
>> -	int ctx_size = sec_ctx->sadb_x_ctx_len;
>> -
>> -	uctx = kmalloc((sizeof(*uctx)+ctx_size), GFP_KERNEL);
>> -
>> -	if (!uctx)
>> -		return NULL;
>> +	struct xfrm_user_sec_ctx *uctx = (struct xfrm_user_sec_ctx *)sec_ctx;
>>
>>   	uctx->len = pfkey_sec_ctx_len(sec_ctx);
>> -	uctx->exttype = sec_ctx->sadb_x_sec_exttype;
>> -	uctx->ctx_doi = sec_ctx->sadb_x_ctx_doi;
>> -	uctx->ctx_alg = sec_ctx->sadb_x_ctx_alg;
>> -	uctx->ctx_len = sec_ctx->sadb_x_ctx_len;
>> -	memcpy(uctx + 1, sec_ctx + 1,
>> -	       uctx->ctx_len);
>> -
>>   	return uctx;
>>   }
>
> The fact that you are now changing sadb_x_sec_ctx->sadb_x_sec_len whenever
> pfkey_sadb2xfrm_user_sec_ctx() is called raises an eyebrow.  Can you elaborate
> on why this is not a problem?
>
Thanks for your attention, Paul.

sadb_x_sec_ctx is extra headers passed down from user space, the usage of
of this data structure falls down to one of pfkey_funcs function only for
one time, more specifically speaking, it's only used by SELINUX for security
checking for each operation. In other words, sadb_x_sec_ctx involves with a
one shot business here. So the original codes seems do a lots of extra job
which could easily be avoid using casting operation.

-- 
浮沉随浪只记今朝笑

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