| Message ID | 1479238801-13053-3-git-send-email-william.c.roberts@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On 11/15/2016 02:40 PM, william.c.roberts@intel.com wrote: > From: Stephen Smalley <sds@tycho.nsa.gov> > > 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 reimplimation of 6201bb5e2 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 | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > index 004a029..ef04908 100644 > --- a/libsepol/src/expand.c > +++ b/libsepol/src/expand.c > @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t * state, > static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > avtab_t * avtab, avtab_key_t * key, > cond_av_list_t ** cond, > - av_extended_perms_t *xperms) > + av_extended_perms_t *xperms, > + uint32_t init_data) > { > avtab_ptr_t node; > avtab_datum_t avdatum; > @@ -1640,6 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > > if (!node) { > memset(&avdatum, 0, sizeof avdatum); > + avdatum.data = init_data; > /* this is used to get the node - insertion is actually unique */ > node = avtab_insert_nonunique(avtab, key, &avdatum); > if (!node) { > @@ -1750,7 +1752,7 @@ static int expand_terule_helper(sepol_handle_t * handle, > return EXPAND_RULE_CONFLICT; > } > > - node = find_avtab_node(handle, avtab, &avkey, cond, NULL); > + node = find_avtab_node(handle, avtab, &avkey, cond, NULL, 0); > if (!node) > return -1; > if (enabled) { > @@ -1824,7 +1826,15 @@ static int expand_avrule_helper(sepol_handle_t * handle, > avkey.target_class = cur->tclass; > avkey.specified = spec; > > - node = find_avtab_node(handle, avtab, &avkey, cond, extended_perms); > + node = find_avtab_node(handle, avtab, &avkey, cond, > + extended_perms, > + /* > + * dontaudit rules need to be > + * initialized on creation, others can > + * be 0. > + */ > + specified & AVRULE_DONTAUDIT > + ? ~cur->data : 0); There seems to be a lack of symmetry here. Either we should initialize them all to the proper value (including AUDITDENY, which I think might be broken too, although no one uses it anymore since the transition to DONTAUDIT), or we should initialize the ones that are ORed to 0 and the ones that are ANDed to ~0 and then let the subsequent OR'ing or AND'ing adjust appropriately. > if (!node) > return EXPAND_RULE_ERROR; > if (enabled) { > @@ -1850,10 +1860,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) { >
On 11/15/2016 03:11 PM, Stephen Smalley wrote: > On 11/15/2016 02:40 PM, william.c.roberts@intel.com wrote: >> From: Stephen Smalley <sds@tycho.nsa.gov> Also, you don't have to keep me as the author as the patch is a rewrite after a revert. >> >> 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 reimplimation of 6201bb5e2 that avoids the cumbersome >> pointer assignments on alloced. s/reimplimation/reimplementation/ s/6201bb532/commit 6201bb5e258e2b5bcc04d502d6fbc05c69d21d71 ("libsepol: fix checkpolicy dontaudit compiler bug")/ >> >> Reported-by: Nick Kralevich <nnk@google.com> >> Signed-off-by: William Roberts <william.c.roberts@intel.com> >> --- >> libsepol/src/expand.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c >> index 004a029..ef04908 100644 >> --- a/libsepol/src/expand.c >> +++ b/libsepol/src/expand.c >> @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t * state, >> static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, >> avtab_t * avtab, avtab_key_t * key, >> cond_av_list_t ** cond, >> - av_extended_perms_t *xperms) >> + av_extended_perms_t *xperms, >> + uint32_t init_data) >> { >> avtab_ptr_t node; >> avtab_datum_t avdatum; >> @@ -1640,6 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, >> >> if (!node) { >> memset(&avdatum, 0, sizeof avdatum); >> + avdatum.data = init_data; >> /* this is used to get the node - insertion is actually unique */ >> node = avtab_insert_nonunique(avtab, key, &avdatum); >> if (!node) { >> @@ -1750,7 +1752,7 @@ static int expand_terule_helper(sepol_handle_t * handle, >> return EXPAND_RULE_CONFLICT; >> } >> >> - node = find_avtab_node(handle, avtab, &avkey, cond, NULL); >> + node = find_avtab_node(handle, avtab, &avkey, cond, NULL, 0); >> if (!node) >> return -1; >> if (enabled) { >> @@ -1824,7 +1826,15 @@ static int expand_avrule_helper(sepol_handle_t * handle, >> avkey.target_class = cur->tclass; >> avkey.specified = spec; >> >> - node = find_avtab_node(handle, avtab, &avkey, cond, extended_perms); >> + node = find_avtab_node(handle, avtab, &avkey, cond, >> + extended_perms, >> + /* >> + * dontaudit rules need to be >> + * initialized on creation, others can >> + * be 0. >> + */ >> + specified & AVRULE_DONTAUDIT >> + ? ~cur->data : 0); > > There seems to be a lack of symmetry here. Either we should initialize > them all to the proper value (including AUDITDENY, which I think might > be broken too, although no one uses it anymore since the transition to > DONTAUDIT), or we should initialize the ones that are ORed to 0 and the > ones that are ANDed to ~0 and then let the subsequent OR'ing or AND'ing > adjust appropriately. > >> if (!node) >> return EXPAND_RULE_ERROR; >> if (enabled) { >> @@ -1850,10 +1860,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) { >> >
On Nov 15, 2016 12:12, "Stephen Smalley" <sds@tycho.nsa.gov> wrote: > > On 11/15/2016 03:11 PM, Stephen Smalley wrote: > > On 11/15/2016 02:40 PM, william.c.roberts@intel.com wrote: > >> From: Stephen Smalley <sds@tycho.nsa.gov> > > Also, you don't have to keep me as the author as the patch is a rewrite > after a revert. I didn't realize -C kept the author on git commit. > > >> > >> 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 reimplimation of 6201bb5e2 that avoids the cumbersome > >> pointer assignments on alloced. > > s/reimplimation/reimplementation/ > s/6201bb532/commit 6201bb5e258e2b5bcc04d502d6fbc05c69d21d71 ("libsepol: > fix checkpolicy dontaudit compiler bug")/ > > >> > >> Reported-by: Nick Kralevich <nnk@google.com> > >> Signed-off-by: William Roberts <william.c.roberts@intel.com> > >> --- > >> libsepol/src/expand.c | 21 ++++++++++++++------- > >> 1 file changed, 14 insertions(+), 7 deletions(-) > >> > >> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > >> index 004a029..ef04908 100644 > >> --- a/libsepol/src/expand.c > >> +++ b/libsepol/src/expand.c > >> @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t * state, > >> static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > >> avtab_t * avtab, avtab_key_t * key, > >> cond_av_list_t ** cond, > >> - av_extended_perms_t *xperms) > >> + av_extended_perms_t *xperms, > >> + uint32_t init_data) > >> { > >> avtab_ptr_t node; > >> avtab_datum_t avdatum; > >> @@ -1640,6 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > >> > >> if (!node) { > >> memset(&avdatum, 0, sizeof avdatum); > >> + avdatum.data = init_data; > >> /* this is used to get the node - insertion is actually unique */ > >> node = avtab_insert_nonunique(avtab, key, &avdatum); > >> if (!node) { > >> @@ -1750,7 +1752,7 @@ static int expand_terule_helper(sepol_handle_t * handle, > >> return EXPAND_RULE_CONFLICT; > >> } > >> > >> - node = find_avtab_node(handle, avtab, &avkey, cond, NULL); > >> + node = find_avtab_node(handle, avtab, &avkey, cond, NULL, 0); > >> if (!node) > >> return -1; > >> if (enabled) { > >> @@ -1824,7 +1826,15 @@ static int expand_avrule_helper(sepol_handle_t * handle, > >> avkey.target_class = cur->tclass; > >> avkey.specified = spec; > >> > >> - node = find_avtab_node(handle, avtab, &avkey, cond, extended_perms); > >> + node = find_avtab_node(handle, avtab, &avkey, cond, > >> + extended_perms, > >> + /* > >> + * dontaudit rules need to be > >> + * initialized on creation, others can > >> + * be 0. > >> + */ > >> + specified & AVRULE_DONTAUDIT > >> + ? ~cur->data : 0); > > > > There seems to be a lack of symmetry here. Either we should initialize > > them all to the proper value (including AUDITDENY, which I think might > > be broken too, although no one uses it anymore since the transition to > > DONTAUDIT), or we should initialize the ones that are ORed to 0 and the > > ones that are ANDed to ~0 and then let the subsequent OR'ing or AND'ing > > adjust appropriately. > > > >> if (!node) > >> return EXPAND_RULE_ERROR; > >> if (enabled) { > >> @@ -1850,10 +1860,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) { > >> > > > > _______________________________________________ > 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.
On Nov 15, 2016 12:09, "Stephen Smalley" <sds@tycho.nsa.gov> wrote: > > On 11/15/2016 02:40 PM, william.c.roberts@intel.com wrote: > > From: Stephen Smalley <sds@tycho.nsa.gov> > > > > 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 reimplimation of 6201bb5e2 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 | 21 ++++++++++++++------- > > 1 file changed, 14 insertions(+), 7 deletions(-) > > > > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > > index 004a029..ef04908 100644 > > --- a/libsepol/src/expand.c > > +++ b/libsepol/src/expand.c > > @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t * state, > > static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > > avtab_t * avtab, avtab_key_t * key, > > cond_av_list_t ** cond, > > - av_extended_perms_t *xperms) > > + av_extended_perms_t *xperms, > > + uint32_t init_data) > > { > > avtab_ptr_t node; > > avtab_datum_t avdatum; > > @@ -1640,6 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > > > > if (!node) { > > memset(&avdatum, 0, sizeof avdatum); > > + avdatum.data = init_data; > > /* this is used to get the node - insertion is actually unique */ > > node = avtab_insert_nonunique(avtab, key, &avdatum); > > if (!node) { > > @@ -1750,7 +1752,7 @@ static int expand_terule_helper(sepol_handle_t * handle, > > return EXPAND_RULE_CONFLICT; > > } > > > > - node = find_avtab_node(handle, avtab, &avkey, cond, NULL); > > + node = find_avtab_node(handle, avtab, &avkey, cond, NULL, 0); > > if (!node) > > return -1; > > if (enabled) { > > @@ -1824,7 +1826,15 @@ static int expand_avrule_helper(sepol_handle_t * handle, > > avkey.target_class = cur->tclass; > > avkey.specified = spec; > > > > - node = find_avtab_node(handle, avtab, &avkey, cond, extended_perms); > > + node = find_avtab_node(handle, avtab, &avkey, cond, > > + extended_perms, > > + /* > > + * dontaudit rules need to be > > + * initialized on creation, others can > > + * be 0. > > + */ > > + specified & AVRULE_DONTAUDIT > > + ? ~cur->data : 0); > > There seems to be a lack of symmetry here. Either we should initialize > them all to the proper value (including AUDITDENY, which I think might > be broken too, although no one uses it anymore since the transition to > DONTAUDIT), or we should initialize the ones that are ORed to 0 and the > ones that are ANDed to ~0 and then let the subsequent OR'ing or AND'ing > adjust appropriately. I noticed that and thought of doing that, so I'm OK with that. I'll send a v2. > > > if (!node) > > return EXPAND_RULE_ERROR; > > if (enabled) { > > @@ -1850,10 +1860,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) { > > > > _______________________________________________ > 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.
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c index 004a029..ef04908 100644 --- a/libsepol/src/expand.c +++ b/libsepol/src/expand.c @@ -1604,7 +1604,8 @@ static int expand_range_trans(expand_state_t * state, static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, avtab_t * avtab, avtab_key_t * key, cond_av_list_t ** cond, - av_extended_perms_t *xperms) + av_extended_perms_t *xperms, + uint32_t init_data) { avtab_ptr_t node; avtab_datum_t avdatum; @@ -1640,6 +1641,7 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, if (!node) { memset(&avdatum, 0, sizeof avdatum); + avdatum.data = init_data; /* this is used to get the node - insertion is actually unique */ node = avtab_insert_nonunique(avtab, key, &avdatum); if (!node) { @@ -1750,7 +1752,7 @@ static int expand_terule_helper(sepol_handle_t * handle, return EXPAND_RULE_CONFLICT; } - node = find_avtab_node(handle, avtab, &avkey, cond, NULL); + node = find_avtab_node(handle, avtab, &avkey, cond, NULL, 0); if (!node) return -1; if (enabled) { @@ -1824,7 +1826,15 @@ static int expand_avrule_helper(sepol_handle_t * handle, avkey.target_class = cur->tclass; avkey.specified = spec; - node = find_avtab_node(handle, avtab, &avkey, cond, extended_perms); + node = find_avtab_node(handle, avtab, &avkey, cond, + extended_perms, + /* + * dontaudit rules need to be + * initialized on creation, others can + * be 0. + */ + specified & AVRULE_DONTAUDIT + ? ~cur->data : 0); if (!node) return EXPAND_RULE_ERROR; if (enabled) { @@ -1850,10 +1860,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) {