Refactor expansion of avtab

PATCH In review Joshua Brindle 2 participants 4 messages Jul 25, 2006 – Jul 27, 2006 SELinux security module

Share & tools

Joshua Brindle

author

2006-07-25 14:56 UTC #0 lore raw

PATCH 2/2 Refactor expansion of avtab

The patch adds a new function called expand_module_avrules that creates an expand_state object and expands the avrules (including the neverallows). This function permits external users of libsepol to expand the avrules into the same policy. We refactored and created a static function called expand_avrule_decls since its functionality is needed in the original expand_module and the new expand_module_avrules functions.

libsepol/include/sepol/policydb/expand.h +3 -0

diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' -x '*.o' -x '*.lo' ../../../trunk/libsepol/include/sepol/policydb/expand.h ./include/sepol/policydb/expand.h
@@ -29,6 +29,9 @@
 #include <sepol/handle.h>
 #include <sepol/policydb/conditional.h>
 
+extern int expand_module_avrules(sepol_handle_t *handle, policydb_t *base,
+				policydb_t *out, uint32_t *typemap, 
+				int verbose, int expand_neverallow);
 extern int expand_module(sepol_handle_t * handle,
 			 policydb_t * base, policydb_t * out,
 			 int verbose, int check);

libsepol/src/expand.c +69 ~~-40~~

121 lines not on this page · raw message

--
This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe selinux" without quotes as the message.

Karl MacMillan

2006-07-25 16:16 UTC #1 lore raw

libsepol/include/sepol/policydb/expand.h line 34

Quoted 10 lines · Joshua Brindle

diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej' -x '*.svn*' -x '*.swp' -x '*.o' -x '*.lo' ../../../trunk/libsepol/include/sepol/policydb/expand.h ./include/sepol/policydb/expand.h
libsepol/include/sepol/policydb/expand.h +3 -0
@@ -29,6 +29,9 @@
 #include <sepol/handle.h>
 #include <sepol/policydb/conditional.h>
 
+extern int expand_module_avrules(sepol_handle_t *handle, policydb_t *base,
+				policydb_t *out, uint32_t *typemap, 
+				int verbose, int expand_neverallow);

Poorly named function - are neverallows av rules or not? If they are not the function needs a more generic name. This is continuing the confusing practice of sometimes calling just allow and audit rules av rules and sometimes using it to mean more rule types.

This can be in place or out of place (i.e., out can be the same as base)? A comment describing how this function can be used is needed, including the fact that the typemap must be "special" for an in-place expand, correct?

Object classes and permissions will never need to be mapped for an out of place expansion?

libsepol/include/sepol/policydb/expand.h

Quoted 10 lines

libsepol/include/sepol/policydb/expand.h +3 -3

-		/* copy conditional rules */
-		if (cond_node_copy(&state, decl->cond_list))
-			goto cleanup;
+	if (expand_avrule_decls(&state) < 0) {
+		ERR(handle, "Error during expand");
+		goto cleanup;

libsepol/include/sepol/policydb/expand.h +0 -0

 	}
 
 	/* copy constraints */

The refactoring is nice even without the exported function.

Karl

Joshua Brindle

author

2006-07-25 16:55 UTC #2 lore raw

Quoted 18 lines

From: Karl MacMillan [mailto:[email protected]]
Quoted 1 line · Joshua Brindle
diff -urpN -x 'Change*' -x entries -x '*.orig' -x '*.rej'
-x '*.svn*' -x '*.swp' -x '*.o' -x '*.lo' ../../../trunk/libsepol/include/sepol/policydb/expand.h ./include/sepol/policydb/expand.h
Quoted 1 line
--- trunk/libsepol/include/sepol/policydb/expand.h
2006-07-13 10:19:14.000000000 -0400
Quoted 1 line
+++ trunk/libsepol/include/sepol/policydb/expand.h
2006-07-19 13:04:03.000000000 -0400
libsepol/include/sepol/policydb/expand.h line 32
Quoted 5 lines
libsepol/include/sepol/policydb/expand.h +1 -0
@@ -29,6 +29,9 @@
 #include <sepol/handle.h>
 #include <sepol/policydb/conditional.h>
 
+extern int expand_module_avrules(sepol_handle_t *handle, 
policydb_t *base,

Quoted 9 lines

libsepol/include/sepol/policydb/expand.h line 34
Quoted 2 lines
libsepol/include/sepol/policydb/expand.h +2 -0
+				policydb_t *out, uint32_t *typemap, 
+				int verbose, int expand_neverallow);
Poorly named function - are neverallows av rules or not? If they are not the function needs a more generic name. This is continuing the confusing practice of sometimes calling just allow and audit rules av rules and sometimes using it to mean more rule types.

Have any suggestions? We couldn't think of a really good name either.

Quoted 5 lines

This can be in place or out of place (i.e., out can be the same as base)? A comment describing how this function can be used is needed, including the fact that the typemap must be "special" for an in-place expand, correct?

Either, it is out of place for the current usage and in place for setools. Talking about a special typemap is out of context here. Maybe more comments are needed. No need to ditch this patch though, we can apply some comments on top of it.

Quoted 11 lines

Object classes and permissions will never need to be mapped for an out of place expansion?
Quoted 8 lines
libsepol/src/expand.c +3 -3
-		/* copy conditional rules */
-		if (cond_node_copy(&state, decl->cond_list))
-			goto cleanup;
+	if (expand_avrule_decls(&state) < 0) {
+		ERR(handle, "Error during expand");
+		goto cleanup;
}

Quoted 5 lines

Quoted 2 lines
/* copy constraints */
The refactoring is nice even without the exported function.

Karl MacMillan

2006-07-27 14:36 UTC #3 lore raw

Quoted 10 lines · Joshua Brindle

Quoted 6 lines
From: Karl MacMillan [mailto:[email protected]]
Poorly named function - are neverallows av rules or not? If they are not the function needs a more generic name. This is continuing the confusing practice of sometimes calling just allow and audit rules av rules and sometimes using it to mean more rule types.
Have any suggestions? We couldn't think of a really good name either.

I vote we start using avrules to mean allow, audit, and neverallow - i.e., based on their common syntax. That would argue for the define changing in the other patch.

Quoted 10 lines

Quoted 3 lines
This can be in place or out of place (i.e., out can be the same as base)? A comment describing how this function can be used is needed, including the fact that the typemap must be "special" for an in-place expand, correct?
Either, it is out of place for the current usage and in place for setools. Talking about a special typemap is out of context here. Maybe more comments are needed. No need to ditch this patch though, we can apply some comments on top of it.

Why is talking about a specific typemap out of place? Just give the user a hint that if they want to do in-place expansion what the typemap will be. Where is the real documentation for typemap going to go?

Quoted 3 lines

Quoted 3 lines
Object classes and permissions will never need to be mapped for an out of place expansion?

What about this question?

Karl