All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] libsepol users in non-mls policy bugfix
@ 2006-10-30 13:59 Joshua Brindle
  2006-10-30 18:26 ` Karl MacMillan
  0 siblings, 1 reply; 8+ messages in thread
From: Joshua Brindle @ 2006-10-30 13:59 UTC (permalink / raw)
  To: selinux@tycho.nsa.gov; +Cc: Stephen Smalley, Karl MacMillan, Darrel Goeddel

This patch fixes a bug where adding a user in a non-mls module would
cause a segfault at link time.


diff -pruN trunk/libsepol/src/link.c user_mls_fix/libsepol/src/link.c
--- trunk/libsepol/src/link.c	2006-10-28 15:35:26.000000000 -0400
+++ user_mls_fix/libsepol/src/link.c	2006-10-28 15:26:44.000000000 -0400
@@ -994,11 +994,15 @@ static int user_fix_callback(hashtab_key
 		goto cleanup;
 	}
 
-	if (mls_range_convert(&user->range, &new_user->range, mod))
-		goto cleanup;
+	if (mod->policy->mls) {
 
-	if (mls_level_convert(&user->dfltlevel, &new_user->dfltlevel, mod))
-		goto cleanup;
+		if (mls_range_convert(&user->range, &new_user->range, mod))
+			goto cleanup;
+
+		if (mls_level_convert(&user->dfltlevel, &new_user->dfltlevel, mod))
+			goto cleanup;
+
+	}
 
 	return 0;
 



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libsepol users in non-mls policy bugfix
  2006-10-30 13:59 [PATCH] libsepol users in non-mls policy bugfix Joshua Brindle
@ 2006-10-30 18:26 ` Karl MacMillan
  2006-10-30 18:31   ` Joshua Brindle
  0 siblings, 1 reply; 8+ messages in thread
From: Karl MacMillan @ 2006-10-30 18:26 UTC (permalink / raw)
  To: Joshua Brindle; +Cc: selinux@tycho.nsa.gov, Stephen Smalley, Darrel Goeddel

On Mon, 2006-10-30 at 08:59 -0500, Joshua Brindle wrote:
> This patch fixes a bug where adding a user in a non-mls module would
> cause a segfault at link time.
> 
> 
> diff -pruN trunk/libsepol/src/link.c user_mls_fix/libsepol/src/link.c
> --- trunk/libsepol/src/link.c	2006-10-28 15:35:26.000000000 -0400
> +++ user_mls_fix/libsepol/src/link.c	2006-10-28 15:26:44.000000000 -0400
> @@ -994,11 +994,15 @@ static int user_fix_callback(hashtab_key
>  		goto cleanup;
>  	}
>  
> -	if (mls_range_convert(&user->range, &new_user->range, mod))
> -		goto cleanup;
> +	if (mod->policy->mls) {
>  
> -	if (mls_level_convert(&user->dfltlevel, &new_user->dfltlevel, mod))
> -		goto cleanup;
> +		if (mls_range_convert(&user->range, &new_user->range, mod))
> +			goto cleanup;
> +
> +		if (mls_level_convert(&user->dfltlevel, &new_user->dfltlevel, mod))
> +			goto cleanup;
> +
> +	}
>  
>  	return 0;
>  
> 

This patch looks ok, but shouldn't we also stop the segfault in
mls_level_convert? The use of assert there looks like it is catching
what should be a runtime error that should cause an error message rather
than bailing.

Otherwise:

Acked by: Karl MacMillan <kmacmillan@mentalrootkit.com>

> 
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] libsepol users in non-mls policy bugfix
  2006-10-30 18:26 ` Karl MacMillan
@ 2006-10-30 18:31   ` Joshua Brindle
  2006-10-30 18:52     ` Darrel Goeddel
  2006-10-30 18:56     ` Karl MacMillan
  0 siblings, 2 replies; 8+ messages in thread
From: Joshua Brindle @ 2006-10-30 18:31 UTC (permalink / raw)
  To: Karl MacMillan; +Cc: selinux, Stephen Smalley, Darrel Goeddel

> From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com] 
> 
> On Mon, 2006-10-30 at 08:59 -0500, Joshua Brindle wrote:
> > This patch fixes a bug where adding a user in a non-mls 
> module would 
> > cause a segfault at link time.
> > 
> > 
> > diff -pruN trunk/libsepol/src/link.c 
> user_mls_fix/libsepol/src/link.c
> > --- trunk/libsepol/src/link.c	2006-10-28 
> 15:35:26.000000000 -0400
> > +++ user_mls_fix/libsepol/src/link.c	2006-10-28 
> 15:26:44.000000000 -0400
> > @@ -994,11 +994,15 @@ static int user_fix_callback(hashtab_key
> >  		goto cleanup;
> >  	}
> >  
> > -	if (mls_range_convert(&user->range, &new_user->range, mod))
> > -		goto cleanup;
> > +	if (mod->policy->mls) {
> >  
> > -	if (mls_level_convert(&user->dfltlevel, 
> &new_user->dfltlevel, mod))
> > -		goto cleanup;
> > +		if (mls_range_convert(&user->range, 
> &new_user->range, mod))
> > +			goto cleanup;
> > +
> > +		if (mls_level_convert(&user->dfltlevel, 
> &new_user->dfltlevel, mod))
> > +			goto cleanup;
> > +
> > +	}
> >  
> >  	return 0;
> >  
> > 
> 
> This patch looks ok, but shouldn't we also stop the segfault 
> in mls_level_convert? The use of assert there looks like it 
> is catching what should be a runtime error that should cause 
> an error message rather than bailing.
> 
> Otherwise:

Yea, I tried thinking of a more elegant way to do it and couldn't come
up with anything, any suggestions?

Clearly those assertions only apply to the MLS case, and we caught a
plain bug because of them, I think they are not user errors.

> 
> Acked by: Karl MacMillan <kmacmillan@mentalrootkit.com>
> 
> > 
> > 
> > --
> > This message was distributed to subscribers of the selinux 
> mailing list.
> > If you no longer wish to subscribe, send mail to 
> > majordomo@tycho.nsa.gov with the words "unsubscribe 
> selinux" without quotes as the message.
> 
> 


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libsepol users in non-mls policy bugfix
  2006-10-30 18:31   ` Joshua Brindle
@ 2006-10-30 18:52     ` Darrel Goeddel
  2006-10-30 18:56     ` Karl MacMillan
  1 sibling, 0 replies; 8+ messages in thread
From: Darrel Goeddel @ 2006-10-30 18:52 UTC (permalink / raw)
  To: Joshua Brindle; +Cc: Karl MacMillan, selinux, Stephen Smalley

Joshua Brindle wrote:
>>From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com] 
>>
>>On Mon, 2006-10-30 at 08:59 -0500, Joshua Brindle wrote:
>>
>>>This patch fixes a bug where adding a user in a non-mls 
>>
>>module would 
>>
>>>cause a segfault at link time.
>>>
>>>
>>>diff -pruN trunk/libsepol/src/link.c 
>>
>>user_mls_fix/libsepol/src/link.c
>>
>>>--- trunk/libsepol/src/link.c	2006-10-28 
>>
>>15:35:26.000000000 -0400
>>
>>>+++ user_mls_fix/libsepol/src/link.c	2006-10-28 
>>
>>15:26:44.000000000 -0400
>>
>>>@@ -994,11 +994,15 @@ static int user_fix_callback(hashtab_key
>>> 		goto cleanup;
>>> 	}
>>> 
>>>-	if (mls_range_convert(&user->range, &new_user->range, mod))
>>>-		goto cleanup;
>>>+	if (mod->policy->mls) {
>>> 
>>>-	if (mls_level_convert(&user->dfltlevel, 
>>
>>&new_user->dfltlevel, mod))
>>
>>>-		goto cleanup;
>>>+		if (mls_range_convert(&user->range, 
>>
>>&new_user->range, mod))
>>
>>>+			goto cleanup;
>>>+
>>>+		if (mls_level_convert(&user->dfltlevel, 
>>
>>&new_user->dfltlevel, mod))
>>
>>>+			goto cleanup;
>>>+
>>>+	}
>>> 
>>> 	return 0;
>>> 
>>>
>>
>>This patch looks ok, but shouldn't we also stop the segfault 
>>in mls_level_convert? The use of assert there looks like it 
>>is catching what should be a runtime error that should cause 
>>an error message rather than bailing.
>>
>>Otherwise:
> 
> 
> Yea, I tried thinking of a more elegant way to do it and couldn't come
> up with anything, any suggestions?
> 
> Clearly those assertions only apply to the MLS case, and we caught a
> plain bug because of them, I think they are not user errors.

How about just putting the mls check in mls_level_convert (as it is in
mls_semantic_level_expand())?

diff -ruNp --exclude=.svn selinux-base/libsepol/src/link.c selinux/libsepol/src/link.c
--- selinux-base/libsepol/src/link.c	2006-10-30 12:48:21.000000000 -0600
+++ selinux/libsepol/src/link.c	2006-10-30 12:45:08.000000000 -0600
@@ -832,6 +832,9 @@ static int mls_level_convert(mls_semanti
 {
 	mls_semantic_cat_t *src_cat, *new_cat;
 
+	if (!mod->policy->mls)
+		return 0;
+
 	assert(mod->map[SYM_LEVELS][src->sens - 1]);
 	dst->sens = mod->map[SYM_LEVELS][src->sens - 1];
 

That will catch all callers of the mls conversion functions in the linker.  The
asserts will then only apply in the MLS case as they should be.

-- 

Darrel

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] libsepol users in non-mls policy bugfix
  2006-10-30 18:31   ` Joshua Brindle
  2006-10-30 18:52     ` Darrel Goeddel
@ 2006-10-30 18:56     ` Karl MacMillan
  2006-10-30 21:41       ` Joshua Brindle
  1 sibling, 1 reply; 8+ messages in thread
From: Karl MacMillan @ 2006-10-30 18:56 UTC (permalink / raw)
  To: Joshua Brindle; +Cc: selinux, Stephen Smalley, Darrel Goeddel

On Mon, 2006-10-30 at 13:31 -0500, Joshua Brindle wrote:
> > From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com] 
> > 
> > This patch looks ok, but shouldn't we also stop the segfault 
> > in mls_level_convert? The use of assert there looks like it 
> > is catching what should be a runtime error that should cause 
> > an error message rather than bailing.
> > 
> > Otherwise:
> 
> Yea, I tried thinking of a more elegant way to do it and couldn't come
> up with anything, any suggestions?
> 

You can put the check in mls_level_convert.

> Clearly those assertions only apply to the MLS case, and we caught a
> plain bug because of them, I think they are not user errors.
> 

Yea - I forgot how all of those assertions are used in the link code.
The alternative is below (with some cleanup up error handling that makes
it look larger). Honestly, I don't care which patch.

Karl


Index: libsepol/src/link.c
===================================================================
--- libsepol/src/link.c	(revision 2074)
+++ libsepol/src/link.c	(working copy)
@@ -827,19 +827,24 @@
 	return -1;
 }
 
-static int mls_level_convert(mls_semantic_level_t * src,
-			     mls_semantic_level_t * dst, policy_module_t * mod)
+static int mls_level_convert(mls_semantic_level_t * src, mls_semantic_level_t * dst,
+			     policy_module_t * mod, link_state_t * state)
 {
 	mls_semantic_cat_t *src_cat, *new_cat;
 
+	if (!mod->policy->mls)
+		return 0;
+
 	assert(mod->map[SYM_LEVELS][src->sens - 1]);
 	dst->sens = mod->map[SYM_LEVELS][src->sens - 1];
 
 	for (src_cat = src->cat; src_cat; src_cat = src_cat->next) {
 		new_cat =
 		    (mls_semantic_cat_t *) malloc(sizeof(mls_semantic_cat_t));
-		if (!new_cat)
+		if (!new_cat) {
+			ERR(state->handle, "Out of memory");
 			return -1;
+		}
 		mls_semantic_cat_init(new_cat);
 
 		new_cat->next = dst->cat;
@@ -854,13 +859,16 @@
 	return 0;
 }
 
-static int mls_range_convert(mls_semantic_range_t * src,
-			     mls_semantic_range_t * dst, policy_module_t * mod)
+static int mls_range_convert(mls_semantic_range_t * src, mls_semantic_range_t * dst,
+			     policy_module_t * mod, link_state_t * state)
 {
-	if (mls_level_convert(&src->level[0], &dst->level[0], mod))
-		return -1;
-	if (mls_level_convert(&src->level[1], &dst->level[1], mod))
-		return -1;
+	int ret;
+	ret = mls_level_convert(&src->level[0], &dst->level[0], mod, state);
+	if (ret)
+		return ret;
+	ret = mls_level_convert(&src->level[1], &dst->level[1], mod, state);
+	if (ret)
+		return ret;
 	return 0;
 }
 
@@ -994,10 +1002,10 @@
 		goto cleanup;
 	}
 
-	if (mls_range_convert(&user->range, &new_user->range, mod))
+	if (mls_range_convert(&user->range, &new_user->range, mod, state))
 		goto cleanup;
 
-	if (mls_level_convert(&user->dfltlevel, &new_user->dfltlevel, mod))
+	if (mls_level_convert(&user->dfltlevel, &new_user->dfltlevel, mod, state))
 		goto cleanup;
 
 	return 0;
@@ -1224,7 +1232,7 @@
 			}
 		}
 
-		if (mls_range_convert(&rule->trange, &new_rule->trange, mod))
+		if (mls_range_convert(&rule->trange, &new_rule->trange, mod, state))
 			goto cleanup;
 	}
 	return 0;



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] libsepol users in non-mls policy bugfix
  2006-10-30 18:56     ` Karl MacMillan
@ 2006-10-30 21:41       ` Joshua Brindle
  2006-10-31 18:03         ` Stephen Smalley
  0 siblings, 1 reply; 8+ messages in thread
From: Joshua Brindle @ 2006-10-30 21:41 UTC (permalink / raw)
  To: Karl MacMillan; +Cc: selinux, Stephen Smalley, Darrel Goeddel

> From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com] 
> 
> On Mon, 2006-10-30 at 13:31 -0500, Joshua Brindle wrote:
> > > From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com]
> > > 
> > > This patch looks ok, but shouldn't we also stop the segfault in 
> > > mls_level_convert? The use of assert there looks like it 
> is catching 
> > > what should be a runtime error that should cause an error message 
> > > rather than bailing.
> > > 
> > > Otherwise:
> > 
> > Yea, I tried thinking of a more elegant way to do it and 
> couldn't come 
> > up with anything, any suggestions?
> > 
> 
> You can put the check in mls_level_convert.
> 
> > Clearly those assertions only apply to the MLS case, and we 
> caught a 
> > plain bug because of them, I think they are not user errors.
> > 
> 
> Yea - I forgot how all of those assertions are used in the link code.
> The alternative is below (with some cleanup up error handling 
> that makes it look larger). Honestly, I don't care which patch.
> 

Looks fine aside from the unrelated parts which should have been split
out :)

> Karl
> 
> 
> Index: libsepol/src/link.c
> ===================================================================
> --- libsepol/src/link.c	(revision 2074)
> +++ libsepol/src/link.c	(working copy)
> @@ -827,19 +827,24 @@
>  	return -1;
>  }
>  
> -static int mls_level_convert(mls_semantic_level_t * src,
> -			     mls_semantic_level_t * dst, 
> policy_module_t * mod)
> +static int mls_level_convert(mls_semantic_level_t * src, 
> mls_semantic_level_t * dst,
> +			     policy_module_t * mod, 
> link_state_t * state)
>  {
>  	mls_semantic_cat_t *src_cat, *new_cat;
>  
> +	if (!mod->policy->mls)
> +		return 0;
> +
>  	assert(mod->map[SYM_LEVELS][src->sens - 1]);
>  	dst->sens = mod->map[SYM_LEVELS][src->sens - 1];
>  
>  	for (src_cat = src->cat; src_cat; src_cat = src_cat->next) {
>  		new_cat =
>  		    (mls_semantic_cat_t *) 
> malloc(sizeof(mls_semantic_cat_t));
> -		if (!new_cat)
> +		if (!new_cat) {
> +			ERR(state->handle, "Out of memory");
>  			return -1;
> +		}
>  		mls_semantic_cat_init(new_cat);
>  
>  		new_cat->next = dst->cat;
> @@ -854,13 +859,16 @@
>  	return 0;
>  }
>  
> -static int mls_range_convert(mls_semantic_range_t * src,
> -			     mls_semantic_range_t * dst, 
> policy_module_t * mod)
> +static int mls_range_convert(mls_semantic_range_t * src, 
> mls_semantic_range_t * dst,
> +			     policy_module_t * mod, 
> link_state_t * state)
>  {
> -	if (mls_level_convert(&src->level[0], &dst->level[0], mod))
> -		return -1;
> -	if (mls_level_convert(&src->level[1], &dst->level[1], mod))
> -		return -1;
> +	int ret;
> +	ret = mls_level_convert(&src->level[0], &dst->level[0], 
> mod, state);
> +	if (ret)
> +		return ret;
> +	ret = mls_level_convert(&src->level[1], &dst->level[1], 
> mod, state);
> +	if (ret)
> +		return ret;
>  	return 0;
>  }
>  
> @@ -994,10 +1002,10 @@
>  		goto cleanup;
>  	}
>  
> -	if (mls_range_convert(&user->range, &new_user->range, mod))
> +	if (mls_range_convert(&user->range, &new_user->range, 
> mod, state))
>  		goto cleanup;
>  
> -	if (mls_level_convert(&user->dfltlevel, 
> &new_user->dfltlevel, mod))
> +	if (mls_level_convert(&user->dfltlevel, 
> &new_user->dfltlevel, mod, 
> +state))
>  		goto cleanup;
>  
>  	return 0;
> @@ -1224,7 +1232,7 @@
>  			}
>  		}
>  
> -		if (mls_range_convert(&rule->trange, 
> &new_rule->trange, mod))
> +		if (mls_range_convert(&rule->trange, 
> &new_rule->trange, mod, state))
>  			goto cleanup;
>  	}
>  	return 0;
> 
> 
> 

Acked-By: Joshua Brindle <jbrindle@tresys.com>


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] libsepol users in non-mls policy bugfix
  2006-10-30 21:41       ` Joshua Brindle
@ 2006-10-31 18:03         ` Stephen Smalley
  2006-10-31 18:40           ` Joshua Brindle
  0 siblings, 1 reply; 8+ messages in thread
From: Stephen Smalley @ 2006-10-31 18:03 UTC (permalink / raw)
  To: Joshua Brindle; +Cc: Karl MacMillan, selinux, Darrel Goeddel

On Mon, 2006-10-30 at 16:41 -0500, Joshua Brindle wrote:
> > From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com] 
> > 
> > On Mon, 2006-10-30 at 13:31 -0500, Joshua Brindle wrote:
> > > > From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com]
> > > > 
> > > > This patch looks ok, but shouldn't we also stop the segfault in 
> > > > mls_level_convert? The use of assert there looks like it 
> > is catching 
> > > > what should be a runtime error that should cause an error message 
> > > > rather than bailing.
> > > > 
> > > > Otherwise:
> > > 
> > > Yea, I tried thinking of a more elegant way to do it and 
> > couldn't come 
> > > up with anything, any suggestions?
> > > 
> > 
> > You can put the check in mls_level_convert.
> > 
> > > Clearly those assertions only apply to the MLS case, and we 
> > caught a 
> > > plain bug because of them, I think they are not user errors.
> > > 
> > 
> > Yea - I forgot how all of those assertions are used in the link code.
> > The alternative is below (with some cleanup up error handling 
> > that makes it look larger). Honestly, I don't care which patch.
> > 
> 
> Looks fine aside from the unrelated parts which should have been split
> out :)
> 
> > Karl
> > 
> > 
> > Index: libsepol/src/link.c
> > ===================================================================
> > --- libsepol/src/link.c	(revision 2074)
> > +++ libsepol/src/link.c	(working copy)
> > @@ -827,19 +827,24 @@
> >  	return -1;
> >  }
> >  
> > -static int mls_level_convert(mls_semantic_level_t * src,
> > -			     mls_semantic_level_t * dst, 
> > policy_module_t * mod)
> > +static int mls_level_convert(mls_semantic_level_t * src, 
> > mls_semantic_level_t * dst,
> > +			     policy_module_t * mod, 
> > link_state_t * state)
> >  {
> >  	mls_semantic_cat_t *src_cat, *new_cat;
> >  
> > +	if (!mod->policy->mls)
> > +		return 0;
> > +
> >  	assert(mod->map[SYM_LEVELS][src->sens - 1]);
> >  	dst->sens = mod->map[SYM_LEVELS][src->sens - 1];
> >  
> >  	for (src_cat = src->cat; src_cat; src_cat = src_cat->next) {
> >  		new_cat =
> >  		    (mls_semantic_cat_t *) 
> > malloc(sizeof(mls_semantic_cat_t));
> > -		if (!new_cat)
> > +		if (!new_cat) {
> > +			ERR(state->handle, "Out of memory");
> >  			return -1;
> > +		}
> >  		mls_semantic_cat_init(new_cat);
> >  
> >  		new_cat->next = dst->cat;
> > @@ -854,13 +859,16 @@
> >  	return 0;
> >  }
> >  
> > -static int mls_range_convert(mls_semantic_range_t * src,
> > -			     mls_semantic_range_t * dst, 
> > policy_module_t * mod)
> > +static int mls_range_convert(mls_semantic_range_t * src, 
> > mls_semantic_range_t * dst,
> > +			     policy_module_t * mod, 
> > link_state_t * state)
> >  {
> > -	if (mls_level_convert(&src->level[0], &dst->level[0], mod))
> > -		return -1;
> > -	if (mls_level_convert(&src->level[1], &dst->level[1], mod))
> > -		return -1;
> > +	int ret;
> > +	ret = mls_level_convert(&src->level[0], &dst->level[0], 
> > mod, state);
> > +	if (ret)
> > +		return ret;
> > +	ret = mls_level_convert(&src->level[1], &dst->level[1], 
> > mod, state);
> > +	if (ret)
> > +		return ret;
> >  	return 0;
> >  }
> >  
> > @@ -994,10 +1002,10 @@
> >  		goto cleanup;
> >  	}
> >  
> > -	if (mls_range_convert(&user->range, &new_user->range, mod))
> > +	if (mls_range_convert(&user->range, &new_user->range, 
> > mod, state))
> >  		goto cleanup;
> >  
> > -	if (mls_level_convert(&user->dfltlevel, 
> > &new_user->dfltlevel, mod))
> > +	if (mls_level_convert(&user->dfltlevel, 
> > &new_user->dfltlevel, mod, 
> > +state))
> >  		goto cleanup;
> >  
> >  	return 0;
> > @@ -1224,7 +1232,7 @@
> >  			}
> >  		}
> >  
> > -		if (mls_range_convert(&rule->trange, 
> > &new_rule->trange, mod))
> > +		if (mls_range_convert(&rule->trange, 
> > &new_rule->trange, mod, state))
> >  			goto cleanup;
> >  	}
> >  	return 0;
> > 
> > 
> > 
> 
> Acked-By: Joshua Brindle <jbrindle@tresys.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] libsepol users in non-mls policy bugfix
  2006-10-31 18:03         ` Stephen Smalley
@ 2006-10-31 18:40           ` Joshua Brindle
  0 siblings, 0 replies; 8+ messages in thread
From: Joshua Brindle @ 2006-10-31 18:40 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Karl MacMillan, selinux, Darrel Goeddel

> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] 
> 
> On Mon, 2006-10-30 at 16:41 -0500, Joshua Brindle wrote:
> > > From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com]
> > > 
> > > On Mon, 2006-10-30 at 13:31 -0500, Joshua Brindle wrote:
> > > > > From: Karl MacMillan [mailto:kmacmillan@mentalrootkit.com]
> > > > > 
> > > > > This patch looks ok, but shouldn't we also stop the 
> segfault in 
> > > > > mls_level_convert? The use of assert there looks like it
> > > is catching
> > > > > what should be a runtime error that should cause an error 
> > > > > message rather than bailing.
> > > > > 
> > > > > Otherwise:
> > > > 
> > > > Yea, I tried thinking of a more elegant way to do it and
> > > couldn't come
> > > > up with anything, any suggestions?
> > > > 
> > > 
> > > You can put the check in mls_level_convert.
> > > 
> > > > Clearly those assertions only apply to the MLS case, and we
> > > caught a
> > > > plain bug because of them, I think they are not user errors.
> > > > 
> > > 
> > > Yea - I forgot how all of those assertions are used in 
> the link code.
> > > The alternative is below (with some cleanup up error 
> handling that 
> > > makes it look larger). Honestly, I don't care which patch.
> > > 
> > 
> > Looks fine aside from the unrelated parts which should have 
> been split 
> > out :)
> > 
> > > Karl
> > > 
> > > 
> > > Index: libsepol/src/link.c
> > > 
> ===================================================================
> > > --- libsepol/src/link.c	(revision 2074)
> > > +++ libsepol/src/link.c	(working copy)
> > > @@ -827,19 +827,24 @@
> > >  	return -1;
> > >  }
> > >  
> > > -static int mls_level_convert(mls_semantic_level_t * src,
> > > -			     mls_semantic_level_t * dst, 
> > > policy_module_t * mod)
> > > +static int mls_level_convert(mls_semantic_level_t * src,
> > > mls_semantic_level_t * dst,
> > > +			     policy_module_t * mod,
> > > link_state_t * state)
> > >  {
> > >  	mls_semantic_cat_t *src_cat, *new_cat;
> > >  
> > > +	if (!mod->policy->mls)
> > > +		return 0;
> > > +
> > >  	assert(mod->map[SYM_LEVELS][src->sens - 1]);
> > >  	dst->sens = mod->map[SYM_LEVELS][src->sens - 1];
> > >  
> > >  	for (src_cat = src->cat; src_cat; src_cat = src_cat->next) {
> > >  		new_cat =
> > >  		    (mls_semantic_cat_t *)
> > > malloc(sizeof(mls_semantic_cat_t));
> > > -		if (!new_cat)
> > > +		if (!new_cat) {
> > > +			ERR(state->handle, "Out of memory");
> > >  			return -1;
> > > +		}
> > >  		mls_semantic_cat_init(new_cat);
> > >  
> > >  		new_cat->next = dst->cat;
> > > @@ -854,13 +859,16 @@
> > >  	return 0;
> > >  }
> > >  
> > > -static int mls_range_convert(mls_semantic_range_t * src,
> > > -			     mls_semantic_range_t * dst, 
> > > policy_module_t * mod)
> > > +static int mls_range_convert(mls_semantic_range_t * src,
> > > mls_semantic_range_t * dst,
> > > +			     policy_module_t * mod,
> > > link_state_t * state)
> > >  {
> > > -	if (mls_level_convert(&src->level[0], &dst->level[0], mod))
> > > -		return -1;
> > > -	if (mls_level_convert(&src->level[1], &dst->level[1], mod))
> > > -		return -1;
> > > +	int ret;
> > > +	ret = mls_level_convert(&src->level[0], &dst->level[0],
> > > mod, state);
> > > +	if (ret)
> > > +		return ret;
> > > +	ret = mls_level_convert(&src->level[1], &dst->level[1],
> > > mod, state);
> > > +	if (ret)
> > > +		return ret;
> > >  	return 0;
> > >  }
> > >  
> > > @@ -994,10 +1002,10 @@
> > >  		goto cleanup;
> > >  	}
> > >  
> > > -	if (mls_range_convert(&user->range, &new_user->range, mod))
> > > +	if (mls_range_convert(&user->range, &new_user->range,
> > > mod, state))
> > >  		goto cleanup;
> > >  
> > > -	if (mls_level_convert(&user->dfltlevel, 
> > > &new_user->dfltlevel, mod))
> > > +	if (mls_level_convert(&user->dfltlevel,
> > > &new_user->dfltlevel, mod,
> > > +state))
> > >  		goto cleanup;
> > >  
> > >  	return 0;
> > > @@ -1224,7 +1232,7 @@
> > >  			}
> > >  		}
> > >  
> > > -		if (mls_range_convert(&rule->trange, 
> > > &new_rule->trange, mod))
> > > +		if (mls_range_convert(&rule->trange,
> > > &new_rule->trange, mod, state))
> > >  			goto cleanup;
> > >  	}
> > >  	return 0;
> > > 
> > > 
> > > 
> > 
> > Acked-By: Joshua Brindle <jbrindle@tresys.com>
> 
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> 

Merged as of libsepol 1.15.2


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2006-10-31 18:40 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-10-30 13:59 [PATCH] libsepol users in non-mls policy bugfix Joshua Brindle
2006-10-30 18:26 ` Karl MacMillan
2006-10-30 18:31   ` Joshua Brindle
2006-10-30 18:52     ` Darrel Goeddel
2006-10-30 18:56     ` Karl MacMillan
2006-10-30 21:41       ` Joshua Brindle
2006-10-31 18:03         ` Stephen Smalley
2006-10-31 18:40           ` Joshua Brindle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.