[PATCH net-next] {selinux, af_key} Rework pfkey_sadb2xfrm_user_sec_ctx

[PATCH net-next] {selinux, af_key} Rework pfkey_sadb2xfrm_user_sec_ctx

[Fan Du]

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@windriver.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;
 }
 
@@ -1125,12 +1112,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 	if (sec_ctx != NULL) {
 		struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx);
 
-		if (!uctx)
-			goto out;
-
 		err = security_xfrm_state_alloc(x, uctx);
-		kfree(uctx);
-
 		if (err)
 			goto out;
 	}
@@ -2225,14 +2207,7 @@ static int pfkey_spdadd(struct sock *sk, struct sk_buff *skb, const struct sadb_
 	if (sec_ctx != NULL) {
 		struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx);
 
-		if (!uctx) {
-			err = -ENOBUFS;
-			goto out;
-		}
-
 		err = security_xfrm_policy_alloc(&xp->security, uctx);
-		kfree(uctx);
-
 		if (err)
 			goto out;
 	}
@@ -2329,11 +2304,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa
 	if (sec_ctx != NULL) {
 		struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx);
 
-		if (!uctx)
-			return -ENOMEM;
-
 		err = security_xfrm_policy_alloc(&pol_ctx, uctx);
-		kfree(uctx);
 		if (err)
 			return err;
 	}
@@ -3230,8 +3201,6 @@ static struct xfrm_policy *pfkey_compile_policy(struct sock *sk, int opt,
 			goto out;
 		uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx);
 		*dir = security_xfrm_policy_alloc(&xp->security, uctx);
-		kfree(uctx);
-
 		if (*dir)
 			goto out;
 	}
-- 
1.7.9.5

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

[Paul Moore]

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

-- 
paul moore
www.paul-moore.com

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

[Fan Du]

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

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

[Steffen Klassert]

On Thu, Oct 17, 2013 at 09:34:53AM +0800, Fan Du wrote:
> 
> 
> On 2013年10月16日 23:15, Paul Moore wrote:
> >
> >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.
> 

Since the selinux people have to live with that change in the fist place,
I'd like to see an ack of one of the selinux maintainers before I take
in into ipsec-next, Paul?

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

[Paul Moore]

On Thursday, October 17, 2013 11:51:48 AM Steffen Klassert wrote:
> On Thu, Oct 17, 2013 at 09:34:53AM +0800, Fan Du wrote:
> > On 2013年10月16日 23:15, Paul Moore wrote:
> > >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.
> 
> Since the selinux people have to live with that change in the fist place,
> I'd like to see an ack of one of the selinux maintainers before I take
> in into ipsec-next, Paul?

Well, my earlier concern over modifying the length field probably isn't a 
major concern as was pointed out so I could maybe look the other way on that 
point.  However, while looking a bit closer at the structs and how they are 
used, I noticed that PFKEY structs are all explicitly packed (sadb_x_sec_ctx) 
and the LSM struct is not (xfrm_user_sec_ctx).  I'm not enough of a compiler 
guru across all the different architectures to know if this is significant or 
not given the structure, but it does make me pause.

Any compiler gurus care to weigh in on this?

-- 
paul moore
www.paul-moore.com

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

[David Miller]

From: Fan Du <fan.du@windriver.com>
Date: Wed, 16 Oct 2013 14:15:14 +0800

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

This isn't safe, one structure is packed and the other is not.

Furthermore, unless there is some enormous gain (in this case there
is not) losing the type checking by casting two data structures like
this is undesirable.

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

[Fan Du]

On 2013年10月19日 03:58, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Wed, 16 Oct 2013 14:15:14 +0800
>
>> 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@windriver.com>
>
> This isn't safe, one structure is packed and the other is not.

Might be. No clue why "one structure is packed and the other is not" happens :(
And why not pack the unpacked structure? or more generally does the packed structure
in this case must be packed in this case?(I doubt this.)

> Furthermore, unless there is some enormous gain (in this case there
> is not) losing the type checking by casting two data structures like
> this is undesirable.

Comparing with the hot path optimization, yes this proposal doesn't bring great
performance boosting. The aim of this patch is not the structure casting indeed
but the avoiding kmalloc/memcpy for a PAGE_SIZE string("context" in SELINUX word)
which maps into a ID for security checking against every AF_KEY operation.


-- 
浮沉随浪只记今朝笑

--fan