[PATCH] libsepol/cil: Create new keep field for type attribute sets

[James Carter]

Daniel Cashman <***@android.com> discovered the following:
When using cil_db multiple_decls, the different cil_attribute nodes
all point to the same underlying cil_attribute struct. This leads
to problems, though, when modifying the used value in the struct.
__cil_post_db_attr() changes the value of the field to based on
the output of cil_typeattribute_used(), for use later in
cil_typeattribute_to_policydb and cil_typeattribute_to_bitmap, but
due to the multiple declarations, cil_typeattribute_used() could be
called again by a second node. In this second call, the value used
is the modifed value of CIL_TRUE or CIL_FALSE, not the flags actually
needed. This could result in the field being reset again, to an
incorrect CIL_FALSE value.

Add the field "keep" to struct cil_typeattributeset, set its value
using cil_typeattribute_used(), and use it when determining whether
the attribute is to be kept or if it should be expanded.

Signed-off-by: James Carter <***@tycho.nsa.gov>
---
libsepol/cil/src/cil.c | 1 +
libsepol/cil/src/cil_binary.c | 8 ++++----
libsepol/cil/src/cil_internal.h | 1 +
libsepol/cil/src/cil_policy.c | 2 +-
libsepol/cil/src/cil_post.c | 2 +-
libsepol/cil/src/cil_reset_ast.c | 1 +
6 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index 3fe68af8..5a64c2bc 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -2064,6 +2064,7 @@ void cil_typeattribute_init(struct cil_typeattribute **attr)
(*attr)->expr_list = NULL;
(*attr)->types = NULL;
(*attr)->used = CIL_FALSE;
+ (*attr)->keep = CIL_FALSE;
}

void cil_typeattributeset_init(struct cil_typeattributeset **attrset)
diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index c0ca60f2..431cd9cd 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -567,7 +567,7 @@ int cil_typeattribute_to_policydb(policydb_t *pdb, struct cil_typeattribute *cil
char *key = NULL;
type_datum_t *sepol_attr = NULL;

- if (!cil_attr->used) {
+ if (!cil_attr->keep) {
return SEPOL_OK;
}

@@ -632,7 +632,7 @@ int cil_typeattribute_to_bitmap(policydb_t *pdb, const struct cil_db *db, struct
ebitmap_node_t *tnode;
unsigned int i;

- if (!cil_attr->used) {
+ if (!cil_attr->keep) {
return SEPOL_OK;
}

@@ -1442,7 +1442,7 @@ static int __cil_should_expand_attribute( const struct cil_db *db, struct cil_sy

attr = (struct cil_typeattribute *)datum;

- return !attr->used || (ebitmap_cardinality(attr->types) < db->attrs_expand_size);
+ return !attr->keep || (ebitmap_cardinality(attr->types) < db->attrs_expand_size);
}

int __cil_avrule_to_avtab(policydb_t *pdb, const struct cil_db *db, struct cil_avrule *cil_avrule, cond_node_t *cond_node, enum cil_flavor cond_flavor)
@@ -2525,7 +2525,7 @@ int __cil_constrain_expr_datum_to_sepol_expr(policydb_t *pdb, const struct cil_d
if (rc != SEPOL_OK) {
if (FLAVOR(item->data) == CIL_TYPEATTRIBUTE) {
struct cil_typeattribute *attr = item->data;
- if (!attr->used) {
+ if (!attr->keep) {
rc = 0;
}
}
diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
index 136a0049..8393e391 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -531,6 +531,7 @@ struct cil_typeattribute {
struct cil_list *expr_list;
ebitmap_t *types;
int used; // whether or not this attribute was used in a binary policy rule
+ int keep;
};

struct cil_typeattributeset {
diff --git a/libsepol/cil/src/cil_policy.c b/libsepol/cil/src/cil_policy.c
index 6d4987c4..99eb53c2 100644
--- a/libsepol/cil/src/cil_policy.c
+++ b/libsepol/cil/src/cil_policy.c
@@ -1085,7 +1085,7 @@ static void cil_typeattributes_to_policy(FILE *out, struct cil_list *types, stru
type = i1->data;
cil_list_for_each(i2, attributes) {
attribute = i2->data;
- if (!attribute->used)
+ if (!attribute->keep)
continue;
if (ebitmap_get_bit(attribute->types, type->value)) {
if (first) {
diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index 3e013c97..a2122454 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -1369,7 +1369,7 @@ static int __cil_post_db_attr_helper(struct cil_tree_node *node, uint32_t *finis
rc = __evaluate_type_expression(attr, db);
if (rc != SEPOL_OK) goto exit;
}
- attr->used = cil_typeattribute_used(attr, db);
+ attr->keep = cil_typeattribute_used(attr, db);
break;
}
case CIL_ROLEATTRIBUTE: {
diff --git a/libsepol/cil/src/cil_reset_ast.c b/libsepol/cil/src/cil_reset_ast.c
index 8a13a1c1..43e6b88e 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -186,6 +186,7 @@ static void cil_reset_typeattr(struct cil_typeattribute *attr)
attr->expr_list = NULL;
}
attr->used = CIL_FALSE;
+ attr->keep = CIL_FALSE;
}

static void cil_reset_typeattributeset(struct cil_typeattributeset *tas)

[Stephen Smalley]

Post by James Carter
When using cil_db multiple_decls, the different cil_attribute nodes
all point to the same underlying cil_attribute struct.  This leads
to problems, though, when modifying the used value in the struct.
__cil_post_db_attr() changes the value of the field to based on
the output of cil_typeattribute_used(), for use later in
cil_typeattribute_to_policydb and cil_typeattribute_to_bitmap, but
due to the multiple declarations, cil_typeattribute_used() could be
called again by a second node.  In this second call, the value used
is the modifed value of CIL_TRUE or CIL_FALSE, not the flags actually
needed. This could result in the field being reset again, to an
incorrect CIL_FALSE value.
Add the field "keep" to struct cil_typeattributeset, set its value
using cil_typeattribute_used(), and use it when determining whether
the attribute is to be kept or if it should be expanded.

Thanks, applied.

Dan, let us know if this resolves your issue.

Post by James Carter
---
 libsepol/cil/src/cil.c           | 1 +
 libsepol/cil/src/cil_binary.c    | 8 ++++----
 libsepol/cil/src/cil_internal.h  | 1 +
 libsepol/cil/src/cil_policy.c    | 2 +-
 libsepol/cil/src/cil_post.c      | 2 +-
 libsepol/cil/src/cil_reset_ast.c | 1 +
 6 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index 3fe68af8..5a64c2bc 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -2064,6 +2064,7 @@ void cil_typeattribute_init(struct
cil_typeattribute **attr)
  (*attr)->expr_list = NULL;
  (*attr)->types = NULL;
  (*attr)->used = CIL_FALSE;
+ (*attr)->keep = CIL_FALSE;
 }
 
 void cil_typeattributeset_init(struct cil_typeattributeset
**attrset)
diff --git a/libsepol/cil/src/cil_binary.c
b/libsepol/cil/src/cil_binary.c
index c0ca60f2..431cd9cd 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -567,7 +567,7 @@ int cil_typeattribute_to_policydb(policydb_t
*pdb, struct cil_typeattribute *cil
  char *key = NULL;
  type_datum_t *sepol_attr = NULL;
 
- if (!cil_attr->used) {
+ if (!cil_attr->keep) {
  return SEPOL_OK;
  }
 
@@ -632,7 +632,7 @@ int cil_typeattribute_to_bitmap(policydb_t *pdb,
const struct cil_db *db, struct
  ebitmap_node_t *tnode;
  unsigned int i;
 
- if (!cil_attr->used) {
+ if (!cil_attr->keep) {
  return SEPOL_OK;
  }
 
@@ -1442,7 +1442,7 @@ static int __cil_should_expand_attribute( const
struct cil_db *db, struct cil_sy
 
  attr = (struct cil_typeattribute *)datum;
 
- return !attr->used || (ebitmap_cardinality(attr->types) <
db->attrs_expand_size);
+ return !attr->keep || (ebitmap_cardinality(attr->types) <
db->attrs_expand_size);
 }
 
 int __cil_avrule_to_avtab(policydb_t *pdb, const struct cil_db *db,
struct cil_avrule *cil_avrule, cond_node_t *cond_node, enum
cil_flavor cond_flavor)
@@ -2525,7 +2525,7 @@ int
__cil_constrain_expr_datum_to_sepol_expr(policydb_t *pdb, const
struct cil_d
  if (rc != SEPOL_OK) {
  if (FLAVOR(item->data) ==
CIL_TYPEATTRIBUTE) {
  struct cil_typeattribute
*attr = item->data;
- if (!attr->used) {
+ if (!attr->keep) {
  rc = 0;
  }
  }
diff --git a/libsepol/cil/src/cil_internal.h
b/libsepol/cil/src/cil_internal.h
index 136a0049..8393e391 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -531,6 +531,7 @@ struct cil_typeattribute {
  struct cil_list *expr_list;
  ebitmap_t *types;
  int used; // whether or not this attribute was used
in a binary policy rule
+ int keep;
 };
 
 struct cil_typeattributeset {
diff --git a/libsepol/cil/src/cil_policy.c
b/libsepol/cil/src/cil_policy.c
index 6d4987c4..99eb53c2 100644
--- a/libsepol/cil/src/cil_policy.c
+++ b/libsepol/cil/src/cil_policy.c
@@ -1085,7 +1085,7 @@ static void cil_typeattributes_to_policy(FILE
*out, struct cil_list *types, stru
  type = i1->data;
  cil_list_for_each(i2, attributes) {
  attribute = i2->data;
- if (!attribute->used)
+ if (!attribute->keep)
  continue;
  if (ebitmap_get_bit(attribute->types, type-

Post by James Carter
value)) {

  if (first) {
diff --git a/libsepol/cil/src/cil_post.c
b/libsepol/cil/src/cil_post.c
index 3e013c97..a2122454 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -1369,7 +1369,7 @@ static int __cil_post_db_attr_helper(struct
cil_tree_node *node, uint32_t *finis
  rc = __evaluate_type_expression(attr, db);
  if (rc != SEPOL_OK) goto exit;
  }
- attr->used = cil_typeattribute_used(attr, db);
+ attr->keep = cil_typeattribute_used(attr, db);
  break;
  }
  case CIL_ROLEATTRIBUTE: {
diff --git a/libsepol/cil/src/cil_reset_ast.c
b/libsepol/cil/src/cil_reset_ast.c
index 8a13a1c1..43e6b88e 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -186,6 +186,7 @@ static void cil_reset_typeattr(struct
cil_typeattribute *attr)
  attr->expr_list = NULL;
  }
  attr->used = CIL_FALSE;
+ attr->keep = CIL_FALSE;
 }
 
 static void cil_reset_typeattributeset(struct cil_typeattributeset
*tas)

[Dan Cashman]

Yep, this works too. Thank you!

Post by Stephen Smalley

Post by James Carter
When using cil_db multiple_decls, the different cil_attribute nodes
all point to the same underlying cil_attribute struct. This leads
to problems, though, when modifying the used value in the struct.
__cil_post_db_attr() changes the value of the field to based on
the output of cil_typeattribute_used(), for use later in
cil_typeattribute_to_policydb and cil_typeattribute_to_bitmap, but
due to the multiple declarations, cil_typeattribute_used() could be
called again by a second node. In this second call, the value used
is the modifed value of CIL_TRUE or CIL_FALSE, not the flags actually
needed. This could result in the field being reset again, to an
incorrect CIL_FALSE value.
Add the field "keep" to struct cil_typeattributeset, set its value
using cil_typeattribute_used(), and use it when determining whether
the attribute is to be kept or if it should be expanded.

Thanks, applied.
Dan, let us know if this resolves your issue.

Post by James Carter
---
libsepol/cil/src/cil.c | 1 +
libsepol/cil/src/cil_binary.c | 8 ++++----
libsepol/cil/src/cil_internal.h | 1 +
libsepol/cil/src/cil_policy.c | 2 +-
libsepol/cil/src/cil_post.c | 2 +-
libsepol/cil/src/cil_reset_ast.c | 1 +
6 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
index 3fe68af8..5a64c2bc 100644
--- a/libsepol/cil/src/cil.c
+++ b/libsepol/cil/src/cil.c
@@ -2064,6 +2064,7 @@ void cil_typeattribute_init(struct
cil_typeattribute **attr)
(*attr)->expr_list = NULL;
(*attr)->types = NULL;
(*attr)->used = CIL_FALSE;
+ (*attr)->keep = CIL_FALSE;
}
void cil_typeattributeset_init(struct cil_typeattributeset
**attrset)
diff --git a/libsepol/cil/src/cil_binary.c
b/libsepol/cil/src/cil_binary.c
index c0ca60f2..431cd9cd 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -567,7 +567,7 @@ int cil_typeattribute_to_policydb(policydb_t
*pdb, struct cil_typeattribute *cil
char *key = NULL;
type_datum_t *sepol_attr = NULL;
- if (!cil_attr->used) {
+ if (!cil_attr->keep) {
return SEPOL_OK;
}
@@ -632,7 +632,7 @@ int cil_typeattribute_to_bitmap(policydb_t *pdb,
const struct cil_db *db, struct
ebitmap_node_t *tnode;
unsigned int i;
- if (!cil_attr->used) {
+ if (!cil_attr->keep) {
return SEPOL_OK;
}
@@ -1442,7 +1442,7 @@ static int __cil_should_expand_attribute( const
struct cil_db *db, struct cil_sy
attr = (struct cil_typeattribute *)datum;
- return !attr->used || (ebitmap_cardinality(attr->types) <
db->attrs_expand_size);
+ return !attr->keep || (ebitmap_cardinality(attr->types) <
db->attrs_expand_size);
}
int __cil_avrule_to_avtab(policydb_t *pdb, const struct cil_db *db,
struct cil_avrule *cil_avrule, cond_node_t *cond_node, enum
cil_flavor cond_flavor)
@@ -2525,7 +2525,7 @@ int
__cil_constrain_expr_datum_to_sepol_expr(policydb_t *pdb, const
struct cil_d
if (rc != SEPOL_OK) {
if (FLAVOR(item->data) ==
CIL_TYPEATTRIBUTE) {
struct cil_typeattribute
*attr = item->data;
- if (!attr->used) {
+ if (!attr->keep) {
rc = 0;
}
}
diff --git a/libsepol/cil/src/cil_internal.h
b/libsepol/cil/src/cil_internal.h
index 136a0049..8393e391 100644
--- a/libsepol/cil/src/cil_internal.h
+++ b/libsepol/cil/src/cil_internal.h
@@ -531,6 +531,7 @@ struct cil_typeattribute {
struct cil_list *expr_list;
ebitmap_t *types;
int used; // whether or not this attribute was used
in a binary policy rule
+ int keep;
};
struct cil_typeattributeset {
diff --git a/libsepol/cil/src/cil_policy.c
b/libsepol/cil/src/cil_policy.c
index 6d4987c4..99eb53c2 100644
--- a/libsepol/cil/src/cil_policy.c
+++ b/libsepol/cil/src/cil_policy.c
@@ -1085,7 +1085,7 @@ static void cil_typeattributes_to_policy(FILE
*out, struct cil_list *types, stru
type = i1->data;
cil_list_for_each(i2, attributes) {
attribute = i2->data;
- if (!attribute->used)
+ if (!attribute->keep)
continue;
if (ebitmap_get_bit(attribute->types, type-

Post by James Carter
value)) {

if (first) {
diff --git a/libsepol/cil/src/cil_post.c
b/libsepol/cil/src/cil_post.c
index 3e013c97..a2122454 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -1369,7 +1369,7 @@ static int __cil_post_db_attr_helper(struct
cil_tree_node *node, uint32_t *finis
rc = __evaluate_type_expression(attr, db);
if (rc != SEPOL_OK) goto exit;
}
- attr->used = cil_typeattribute_used(attr, db);
+ attr->keep = cil_typeattribute_used(attr, db);
break;
}
case CIL_ROLEATTRIBUTE: {
diff --git a/libsepol/cil/src/cil_reset_ast.c
b/libsepol/cil/src/cil_reset_ast.c
index 8a13a1c1..43e6b88e 100644
--- a/libsepol/cil/src/cil_reset_ast.c
+++ b/libsepol/cil/src/cil_reset_ast.c
@@ -186,6 +186,7 @@ static void cil_reset_typeattr(struct
cil_typeattribute *attr)
attr->expr_list = NULL;
}
attr->used = CIL_FALSE;
+ attr->keep = CIL_FALSE;
}
static void cil_reset_typeattributeset(struct cil_typeattributeset *tas)