On Wed, Mar 14, 2018 at 4:05 PM, William Roberts <bill.c.roberts@xxxxxxxxx> wrote: > On Wed, Mar 14, 2018 at 3:17 PM, Tri Vo <trong@xxxxxxxxxxx> wrote: >> When Android combines multiple .cil files from system.img and vendor.img >> it's possible to have conflicting expandattribute statements, e.g. >> expandattribute hal_audio true; >> expandattribute hal_audio false; > > Isn't this the policy.conf version? I thought cil files had: > expandtypeattribute, am I wrong here? You'll need to fix checkpolicy too for consistency. See https://github.com/SELinuxProject/selinux/blob/master/checkpolicy/policy_define.c#L1185 > >> >> This change deals with scenario be resolving the value of the >> corresponding expandattribute to false. The rationale behind this >> override is that true is used for reduce run-time lookups, while >> false is used for tests which must pass. > > So in a conflict, it's always forced to false, which prevents expansion. > That seems reasonable. I would imagine this should also update some > document somewhere that describes this behavior, does one exist? > I couldn't find anything, but not sure if it's on some external webpage. > Stephen do you know? > >> --- >> libsepol/cil/src/cil_resolve_ast.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c >> index d1a5ed87..5c66f663 100644 >> --- a/libsepol/cil/src/cil_resolve_ast.c >> +++ b/libsepol/cil/src/cil_resolve_ast.c >> @@ -271,7 +271,6 @@ exit: >> >> int cil_type_used(struct cil_symtab_datum *datum, int used) Change from int to void. The return value is no longer useful. >> { >> - int rc = SEPOL_ERR; >> struct cil_typeattribute *attr = NULL; >> >> if (FLAVOR(datum) == CIL_TYPEATTRIBUTE) { >> @@ -279,16 +278,13 @@ int cil_type_used(struct cil_symtab_datum *datum, int used) >> attr->used |= used; >> if ((attr->used & CIL_ATTR_EXPAND_TRUE) && >> (attr->used & CIL_ATTR_EXPAND_FALSE)) { >> - cil_log(CIL_ERR, "Conflicting use of expandtypeattribute. " >> - "Expandtypeattribute may be set to true or false " >> - "but not both. \n"); >> - goto exit; >> + cil_log(CIL_WARN, "Conflicting use of expandtypeattribute. " >> + "Expandtypeattribute was set to both true or false for %s. " >> + "Resolving to false. \n", attr->datum.name); >> + attr->used ^= CIL_ATTR_EXPAND_TRUE; > > Sure, this saves an operation, but: > > attr->used &= ~CIL_ATTR_EXPAND_TRUE; +1 > > Is less fragile and the usual unset idiom. I won't request this changed unless > either you agree or someone else has the same opinion as me. > > One could always argue that conditional code that relies on the entry > condition is the whole > point of conditional code :-P > >> } >> } >> - >> return SEPOL_OK; >> -exit: >> - return rc; >> } >> >> int cil_resolve_permissionx(struct cil_tree_node *current, struct cil_permissionx *permx, void *extra_args) >> -- >> 2.16.2.804.g6dcf76e118-goog >> >> >
