diff mbox

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

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

Commit Message

Roberts, William C Nov. 16, 2016, 12:42 a.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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

William Roberts Nov. 16, 2016, 2:50 a.m. UTC | #1
On Nov 15, 2016 4:43 PM, <william.c.roberts@intel.com> wrote:
>
> 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 | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 004a029..3e16f58 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1640,6 +1640,11 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t
* handle,
>
>         if (!node) {
>                 memset(&avdatum, 0, sizeof avdatum);
> +               /*
> +                * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |=
for
> +                * others. Initialize the data accordingly.
> +                */
> +               avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
>                 /* this is used to get the node - insertion is actually
unique */
>                 node = avtab_insert_nonunique(avtab, key, &avdatum);
>                 if (!node) {
> @@ -1850,10 +1855,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) {
> --
> 2.7.4
>

This patch has the same sesearch and policy file md5sums compared to master
and after reverting + applying this.

> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to
Selinux-request@tycho.nsa.gov.
Stephen Smalley Nov. 16, 2016, 1:54 p.m. UTC | #2
On 11/15/2016 07:42 PM, william.c.roberts@intel.com wrote:
> 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.

Thanks, applied both patches (revert and reimplementation).

> 
> Reported-by: Nick Kralevich <nnk@google.com>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
>  libsepol/src/expand.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 004a029..3e16f58 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1640,6 +1640,11 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>  
>  	if (!node) {
>  		memset(&avdatum, 0, sizeof avdatum);
> +		/*
> +		 * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
> +		 * others. Initialize the data accordingly.
> +		 */
> +		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;

normally I'd parenthesize this, although it isn't strictly needed.

>  		/* this is used to get the node - insertion is actually unique */
>  		node = avtab_insert_nonunique(avtab, key, &avdatum);
>  		if (!node) {
> @@ -1850,10 +1855,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) {
>
William Roberts Nov. 16, 2016, 2:01 p.m. UTC | #3
On Wed, Nov 16, 2016 at 5:54 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 11/15/2016 07:42 PM, william.c.roberts@intel.com wrote:
>> 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.
>
> Thanks, applied both patches (revert and reimplementation).

These line numbers might not line up, but it was inconsistent, so I didn't know:
1702:    typemap ? typemap[cur->data - 1] : cur->data;
2958: to_be_appended = (cur_state == 1) ?

<snip>
diff mbox

Patch

diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 004a029..3e16f58 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1640,6 +1640,11 @@  static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
 
 	if (!node) {
 		memset(&avdatum, 0, sizeof avdatum);
+		/*
+		 * AUDITDENY, aka DONTAUDIT, are &= assigned, versus |= for
+		 * others. Initialize the data accordingly.
+		 */
+		avdatum.data = key->specified == AVTAB_AUDITDENY ? ~0 : 0;
 		/* this is used to get the node - insertion is actually unique */
 		node = avtab_insert_nonunique(avtab, key, &avdatum);
 		if (!node) {
@@ -1850,10 +1855,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) {