diff mbox

[take2,v5] libsepol: fix checkpolicy dontaudit compiler bug

Message ID 1479252622-20935-1-git-send-email-william.c.roberts@intel.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Roberts, William C Nov. 15, 2016, 11:30 p.m. UTC 
From: William Roberts <william.c.roberts@intel.com>

The combining logic for dontaudit rules was wrong, causing
a dontaudit A B:C *; rule to be clobbered by a dontaudit A B:C p;
rule.

This is a reimplementation of:
commit 6201bb5e258e2b5bcc04d502d6fbc05c69d21d71 ("libsepol:
fix checkpolicy dontaudit compiler bug")
that avoids the cumbersome pointer assignments on alloced.

Reported-by: Nick Kralevich <nnk@google.com>
Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
 libsepol/src/expand.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

William Roberts Nov. 16, 2016, 12:33 a.m. UTC | #1 
<snip>

>                 memset(&avdatum, 0, sizeof avdatum);
> +               /*
> +                * AUDITDENY and DONTAUDIT are &= assigned, versus |= for
> +                * others. Initialize the data accordingly.
> +                */
> +               avdatum.data = (key->specified &
> +                              (AVRULE_AUDITDENY | AVRULE_DONTAUDIT)) ? ~0 : 0;

Nak this, surprising this is working and producing correct output, but
we would want to check
against the AVTAB defines...

This patch, while simple, for some reason is getting the best of me :-P

>                 /* this is used to get the node - insertion is actually unique */
>                 node = avtab_insert_nonunique(avtab, key, &avdatum);
>                 if (!node) {
<snip>
William Roberts Nov. 16, 2016, 2:49 a.m. UTC | #2 
On Nov 15, 2016 4:33 PM, "William Roberts" <bill.c.roberts@gmail.com> wrote:
>
> <snip>
>
> >                 memset(&avdatum, 0, sizeof avdatum);
> > +               /*
> > +                * AUDITDENY and DONTAUDIT are &= assigned, versus |=
for
> > +                * others. Initialize the data accordingly.
> > +                */
> > +               avdatum.data = (key->specified &
> > +                              (AVRULE_AUDITDENY | AVRULE_DONTAUDIT)) ?
~0 : 0;
>
> Nak this, surprising this is working and producing correct output, but

Correct when checking the sesearch output, which makes sense since we're
expecting both classes to be all dontaudit statements.

> we would want to check
> against the AVTAB defines...
>
> This patch, while simple, for some reason is getting the best of me :-P
>
> >                 /* this is used to get the node - insertion is actually
unique */
> >                 node = avtab_insert_nonunique(avtab, key, &avdatum);
> >                 if (!node) {
> <snip>
diff mbox

Patch

diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 004a029..17d2a2a 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1640,6 +1640,12 @@  static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
 
 	if (!node) {
 		memset(&avdatum, 0, sizeof avdatum);
+		/*
+		 * AUDITDENY and DONTAUDIT are &= assigned, versus |= for
+		 * others. Initialize the data accordingly.
+		 */
+		avdatum.data = (key->specified &
+			       (AVRULE_AUDITDENY | AVRULE_DONTAUDIT)) ? ~0 : 0;
 		/* this is used to get the node - insertion is actually unique */
 		node = avtab_insert_nonunique(avtab, key, &avdatum);
 		if (!node) {
@@ -1850,10 +1856,7 @@  static int expand_avrule_helper(sepol_handle_t * handle,
 			 */
 			avdatump->data &= cur->data;
 		} else if (specified & AVRULE_DONTAUDIT) {
-			if (avdatump->data)
-				avdatump->data &= ~cur->data;
-			else
-				avdatump->data = ~cur->data;
+			avdatump->data &= ~cur->data;
 		} else if (specified & AVRULE_XPERMS) {
 			xperms = avdatump->xperms;
 			if (!xperms) {