* [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. @ 2014-04-15 16:27 Richard Haines 2014-04-15 16:40 ` Stephen Smalley 0 siblings, 1 reply; 11+ messages in thread From: Richard Haines @ 2014-04-15 16:27 UTC (permalink / raw) To: selinux The current detection of duplicate rules does not cover the state->out policy and therefore will duplicate filename transition rules if already present. Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> --- libsepol/src/expand.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c index acb6906..e908fdb 100644 --- a/libsepol/src/expand.c +++ b/libsepol/src/expand.c @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r if (cur_trans) continue; + /* Now check if duplicate rule in state->out policy */ + cur_trans = state->out->filename_trans; + + while (cur_trans) { + if (cur_trans->stype == (i + 1) && + cur_trans->ttype == (j + 1) && + cur_trans->tclass == cur_rule->tclass && + !strcmp(cur_trans->name, cur_rule->name)) + break; + cur_trans = cur_trans->next; + } + if (cur_trans) + continue; + new_trans = malloc(sizeof(*new_trans)); if (!new_trans) { ERR(state->handle, "Out of memory!"); -- 1.9.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 16:27 [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy Richard Haines @ 2014-04-15 16:40 ` Stephen Smalley 2014-04-15 18:21 ` Stephen Smalley 0 siblings, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-04-15 16:40 UTC (permalink / raw) To: Richard Haines, selinux, Eric Paris, Daniel J Walsh On 04/15/2014 12:27 PM, Richard Haines wrote: > The current detection of duplicate rules does not cover the state->out > policy and therefore will duplicate filename transition rules if already > present. > > Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> > --- > libsepol/src/expand.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > index acb6906..e908fdb 100644 > --- a/libsepol/src/expand.c > +++ b/libsepol/src/expand.c > @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r > if (cur_trans) > continue; > > + /* Now check if duplicate rule in state->out policy */ > + cur_trans = state->out->filename_trans; > + > + while (cur_trans) { > + if (cur_trans->stype == (i + 1) && > + cur_trans->ttype == (j + 1) && > + cur_trans->tclass == cur_rule->tclass && > + !strcmp(cur_trans->name, cur_rule->name)) > + break; > + cur_trans = cur_trans->next; > + } > + if (cur_trans) > + continue; > + > new_trans = malloc(sizeof(*new_trans)); > if (!new_trans) { > ERR(state->handle, "Out of memory!"); Isn't this effectively a revert of: commit a29f6820c52b60b9028298cde9962dd140bbf9ea Author: Adam Tkac <atkac@redhat.com> Date: Fri May 25 17:55:08 2012 +0200 libsepol: filename_trans: use some better sorting to compare and merge The kernel switched to using a hashtab for filename_trans rules in commit 2463c26d50adc282d19317013ba0ff473823ca47 Author: Eric Paris <eparis@redhat.com> Date: Thu Apr 28 15:11:21 2011 -0400 SELinux: put name based create rules in a hashtable Is there a reason we don't do this in libsepol too? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 16:40 ` Stephen Smalley @ 2014-04-15 18:21 ` Stephen Smalley 2014-04-15 18:27 ` Eric Paris 0 siblings, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-04-15 18:21 UTC (permalink / raw) To: Richard Haines, selinux, Eric Paris, Daniel J Walsh On 04/15/2014 12:40 PM, Stephen Smalley wrote: > On 04/15/2014 12:27 PM, Richard Haines wrote: >> The current detection of duplicate rules does not cover the state->out >> policy and therefore will duplicate filename transition rules if already >> present. >> >> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> >> --- >> libsepol/src/expand.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c >> index acb6906..e908fdb 100644 >> --- a/libsepol/src/expand.c >> +++ b/libsepol/src/expand.c >> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r >> if (cur_trans) >> continue; >> >> + /* Now check if duplicate rule in state->out policy */ >> + cur_trans = state->out->filename_trans; >> + >> + while (cur_trans) { >> + if (cur_trans->stype == (i + 1) && >> + cur_trans->ttype == (j + 1) && >> + cur_trans->tclass == cur_rule->tclass && >> + !strcmp(cur_trans->name, cur_rule->name)) >> + break; >> + cur_trans = cur_trans->next; >> + } >> + if (cur_trans) >> + continue; >> + >> new_trans = malloc(sizeof(*new_trans)); >> if (!new_trans) { >> ERR(state->handle, "Out of memory!"); > > Isn't this effectively a revert of: > > commit a29f6820c52b60b9028298cde9962dd140bbf9ea > Author: Adam Tkac <atkac@redhat.com> > Date: Fri May 25 17:55:08 2012 +0200 > > libsepol: filename_trans: use some better sorting to compare and merge > > The kernel switched to using a hashtab for filename_trans rules in > commit 2463c26d50adc282d19317013ba0ff473823ca47 > Author: Eric Paris <eparis@redhat.com> > Date: Thu Apr 28 15:11:21 2011 -0400 > > SELinux: put name based create rules in a hashtable > > Is there a reason we don't do this in libsepol too? So if I am reading a29f68 correctly, it is completely wrong and should just be reverted. That will fix the duplicate filename transition rules if I am not mistaken. Then separately, we can look at bringing over the switch to using a hashtab that was already done in the kernel and use that to speed up this checking? Comments? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 18:21 ` Stephen Smalley @ 2014-04-15 18:27 ` Eric Paris 2014-04-15 18:34 ` Stephen Smalley 0 siblings, 1 reply; 11+ messages in thread From: Eric Paris @ 2014-04-15 18:27 UTC (permalink / raw) To: Stephen Smalley; +Cc: selinux On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote: > On 04/15/2014 12:40 PM, Stephen Smalley wrote: > > On 04/15/2014 12:27 PM, Richard Haines wrote: > >> The current detection of duplicate rules does not cover the state->out > >> policy and therefore will duplicate filename transition rules if already > >> present. > >> > >> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> > >> --- > >> libsepol/src/expand.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > >> index acb6906..e908fdb 100644 > >> --- a/libsepol/src/expand.c > >> +++ b/libsepol/src/expand.c > >> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r > >> if (cur_trans) > >> continue; > >> > >> + /* Now check if duplicate rule in state->out policy */ > >> + cur_trans = state->out->filename_trans; > >> + > >> + while (cur_trans) { > >> + if (cur_trans->stype == (i + 1) && > >> + cur_trans->ttype == (j + 1) && > >> + cur_trans->tclass == cur_rule->tclass && > >> + !strcmp(cur_trans->name, cur_rule->name)) > >> + break; > >> + cur_trans = cur_trans->next; > >> + } > >> + if (cur_trans) > >> + continue; > >> + > >> new_trans = malloc(sizeof(*new_trans)); > >> if (!new_trans) { > >> ERR(state->handle, "Out of memory!"); > > > > Isn't this effectively a revert of: > > > > commit a29f6820c52b60b9028298cde9962dd140bbf9ea > > Author: Adam Tkac <atkac@redhat.com> > > Date: Fri May 25 17:55:08 2012 +0200 > > > > libsepol: filename_trans: use some better sorting to compare and merge > > > > The kernel switched to using a hashtab for filename_trans rules in > > commit 2463c26d50adc282d19317013ba0ff473823ca47 > > Author: Eric Paris <eparis@redhat.com> > > Date: Thu Apr 28 15:11:21 2011 -0400 > > > > SELinux: put name based create rules in a hashtable > > > > Is there a reason we don't do this in libsepol too? > > So if I am reading a29f68 correctly, it is completely wrong and should > just be reverted. That will fix the duplicate filename transition rules > if I am not mistaken. Then separately, we can look at bringing over the > switch to using a hashtab that was already done in the kernel and use > that to speed up this checking? Comments? I think that's a good idea. The kernel hashtab and this 'fix' were done about the same time. I intended to bring the kernel hashtab over and got distracted and then forgot about it... Shouldn't be a hard thing, and I believe should get us back to having bitwise the same policy in /sys/fs/selinux/policy as we have on disk... -Eric ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 18:27 ` Eric Paris @ 2014-04-15 18:34 ` Stephen Smalley 2014-04-15 18:43 ` Eric Paris 0 siblings, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-04-15 18:34 UTC (permalink / raw) To: Eric Paris; +Cc: selinux On 04/15/2014 02:27 PM, Eric Paris wrote: > On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote: >> On 04/15/2014 12:40 PM, Stephen Smalley wrote: >>> On 04/15/2014 12:27 PM, Richard Haines wrote: >>>> The current detection of duplicate rules does not cover the state->out >>>> policy and therefore will duplicate filename transition rules if already >>>> present. >>>> >>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> >>>> --- >>>> libsepol/src/expand.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c >>>> index acb6906..e908fdb 100644 >>>> --- a/libsepol/src/expand.c >>>> +++ b/libsepol/src/expand.c >>>> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r >>>> if (cur_trans) >>>> continue; >>>> >>>> + /* Now check if duplicate rule in state->out policy */ >>>> + cur_trans = state->out->filename_trans; >>>> + >>>> + while (cur_trans) { >>>> + if (cur_trans->stype == (i + 1) && >>>> + cur_trans->ttype == (j + 1) && >>>> + cur_trans->tclass == cur_rule->tclass && >>>> + !strcmp(cur_trans->name, cur_rule->name)) >>>> + break; >>>> + cur_trans = cur_trans->next; >>>> + } >>>> + if (cur_trans) >>>> + continue; >>>> + >>>> new_trans = malloc(sizeof(*new_trans)); >>>> if (!new_trans) { >>>> ERR(state->handle, "Out of memory!"); >>> >>> Isn't this effectively a revert of: >>> >>> commit a29f6820c52b60b9028298cde9962dd140bbf9ea >>> Author: Adam Tkac <atkac@redhat.com> >>> Date: Fri May 25 17:55:08 2012 +0200 >>> >>> libsepol: filename_trans: use some better sorting to compare and merge >>> >>> The kernel switched to using a hashtab for filename_trans rules in >>> commit 2463c26d50adc282d19317013ba0ff473823ca47 >>> Author: Eric Paris <eparis@redhat.com> >>> Date: Thu Apr 28 15:11:21 2011 -0400 >>> >>> SELinux: put name based create rules in a hashtable >>> >>> Is there a reason we don't do this in libsepol too? >> >> So if I am reading a29f68 correctly, it is completely wrong and should >> just be reverted. That will fix the duplicate filename transition rules >> if I am not mistaken. Then separately, we can look at bringing over the >> switch to using a hashtab that was already done in the kernel and use >> that to speed up this checking? Comments? > > I think that's a good idea. The kernel hashtab and this 'fix' were done > about the same time. I intended to bring the kernel hashtab over and > got distracted and then forgot about it... > > Shouldn't be a hard thing, and I believe should get us back to having > bitwise the same policy in /sys/fs/selinux/policy as we have on disk... Looks like we have some other areas of divergence, e.g. range_tr hashtab conversion, ebitmap optimizations, possibly others. The latter would require a policy binary format change (new version) to make it fully identical. Not sure how well even the hashtab is doing at this point as Fedora has > 150,000 file name transition rules in its policy per sedispol output? That seems a bit excessive. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 18:34 ` Stephen Smalley @ 2014-04-15 18:43 ` Eric Paris 2014-04-15 18:46 ` Stephen Smalley 2014-04-15 19:07 ` Stephen Smalley 0 siblings, 2 replies; 11+ messages in thread From: Eric Paris @ 2014-04-15 18:43 UTC (permalink / raw) To: Stephen Smalley; +Cc: selinux On Tue, 2014-04-15 at 14:34 -0400, Stephen Smalley wrote: > On 04/15/2014 02:27 PM, Eric Paris wrote: > > On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote: > >> On 04/15/2014 12:40 PM, Stephen Smalley wrote: > >>> On 04/15/2014 12:27 PM, Richard Haines wrote: > >>>> The current detection of duplicate rules does not cover the state->out > >>>> policy and therefore will duplicate filename transition rules if already > >>>> present. > >>>> > >>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> > >>>> --- > >>>> libsepol/src/expand.c | 14 ++++++++++++++ > >>>> 1 file changed, 14 insertions(+) > >>>> > >>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > >>>> index acb6906..e908fdb 100644 > >>>> --- a/libsepol/src/expand.c > >>>> +++ b/libsepol/src/expand.c > >>>> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r > >>>> if (cur_trans) > >>>> continue; > >>>> > >>>> + /* Now check if duplicate rule in state->out policy */ > >>>> + cur_trans = state->out->filename_trans; > >>>> + > >>>> + while (cur_trans) { > >>>> + if (cur_trans->stype == (i + 1) && > >>>> + cur_trans->ttype == (j + 1) && > >>>> + cur_trans->tclass == cur_rule->tclass && > >>>> + !strcmp(cur_trans->name, cur_rule->name)) > >>>> + break; > >>>> + cur_trans = cur_trans->next; > >>>> + } > >>>> + if (cur_trans) > >>>> + continue; > >>>> + > >>>> new_trans = malloc(sizeof(*new_trans)); > >>>> if (!new_trans) { > >>>> ERR(state->handle, "Out of memory!"); > >>> > >>> Isn't this effectively a revert of: > >>> > >>> commit a29f6820c52b60b9028298cde9962dd140bbf9ea > >>> Author: Adam Tkac <atkac@redhat.com> > >>> Date: Fri May 25 17:55:08 2012 +0200 > >>> > >>> libsepol: filename_trans: use some better sorting to compare and merge > >>> > >>> The kernel switched to using a hashtab for filename_trans rules in > >>> commit 2463c26d50adc282d19317013ba0ff473823ca47 > >>> Author: Eric Paris <eparis@redhat.com> > >>> Date: Thu Apr 28 15:11:21 2011 -0400 > >>> > >>> SELinux: put name based create rules in a hashtable > >>> > >>> Is there a reason we don't do this in libsepol too? > >> > >> So if I am reading a29f68 correctly, it is completely wrong and should > >> just be reverted. That will fix the duplicate filename transition rules > >> if I am not mistaken. Then separately, we can look at bringing over the > >> switch to using a hashtab that was already done in the kernel and use > >> that to speed up this checking? Comments? > > > > I think that's a good idea. The kernel hashtab and this 'fix' were done > > about the same time. I intended to bring the kernel hashtab over and > > got distracted and then forgot about it... > > > > Shouldn't be a hard thing, and I believe should get us back to having > > bitwise the same policy in /sys/fs/selinux/policy as we have on disk... > > Looks like we have some other areas of divergence, e.g. range_tr hashtab > conversion, ebitmap optimizations, possibly others. The latter would > require a policy binary format change (new version) to make it fully > identical. > > Not sure how well even the hashtab is doing at this point as Fedora has > > 150,000 file name transition rules in its policy per sedispol output? > That seems a bit excessive. My understanding is that MANY of these are because of the expansion of attributes in the source and the parent object. If we could leave these as attributes it would shrink things and incredible amount... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 18:43 ` Eric Paris @ 2014-04-15 18:46 ` Stephen Smalley 2014-04-15 19:07 ` Stephen Smalley 1 sibling, 0 replies; 11+ messages in thread From: Stephen Smalley @ 2014-04-15 18:46 UTC (permalink / raw) To: Eric Paris; +Cc: selinux On 04/15/2014 02:43 PM, Eric Paris wrote: > On Tue, 2014-04-15 at 14:34 -0400, Stephen Smalley wrote: >> On 04/15/2014 02:27 PM, Eric Paris wrote: >>> On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote: >>>> On 04/15/2014 12:40 PM, Stephen Smalley wrote: >>>>> On 04/15/2014 12:27 PM, Richard Haines wrote: >>>>>> The current detection of duplicate rules does not cover the state->out >>>>>> policy and therefore will duplicate filename transition rules if already >>>>>> present. >>>>>> >>>>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> >>>>>> --- >>>>>> libsepol/src/expand.c | 14 ++++++++++++++ >>>>>> 1 file changed, 14 insertions(+) >>>>>> >>>>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c >>>>>> index acb6906..e908fdb 100644 >>>>>> --- a/libsepol/src/expand.c >>>>>> +++ b/libsepol/src/expand.c >>>>>> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r >>>>>> if (cur_trans) >>>>>> continue; >>>>>> >>>>>> + /* Now check if duplicate rule in state->out policy */ >>>>>> + cur_trans = state->out->filename_trans; >>>>>> + >>>>>> + while (cur_trans) { >>>>>> + if (cur_trans->stype == (i + 1) && >>>>>> + cur_trans->ttype == (j + 1) && >>>>>> + cur_trans->tclass == cur_rule->tclass && >>>>>> + !strcmp(cur_trans->name, cur_rule->name)) >>>>>> + break; >>>>>> + cur_trans = cur_trans->next; >>>>>> + } >>>>>> + if (cur_trans) >>>>>> + continue; >>>>>> + >>>>>> new_trans = malloc(sizeof(*new_trans)); >>>>>> if (!new_trans) { >>>>>> ERR(state->handle, "Out of memory!"); >>>>> >>>>> Isn't this effectively a revert of: >>>>> >>>>> commit a29f6820c52b60b9028298cde9962dd140bbf9ea >>>>> Author: Adam Tkac <atkac@redhat.com> >>>>> Date: Fri May 25 17:55:08 2012 +0200 >>>>> >>>>> libsepol: filename_trans: use some better sorting to compare and merge >>>>> >>>>> The kernel switched to using a hashtab for filename_trans rules in >>>>> commit 2463c26d50adc282d19317013ba0ff473823ca47 >>>>> Author: Eric Paris <eparis@redhat.com> >>>>> Date: Thu Apr 28 15:11:21 2011 -0400 >>>>> >>>>> SELinux: put name based create rules in a hashtable >>>>> >>>>> Is there a reason we don't do this in libsepol too? >>>> >>>> So if I am reading a29f68 correctly, it is completely wrong and should >>>> just be reverted. That will fix the duplicate filename transition rules >>>> if I am not mistaken. Then separately, we can look at bringing over the >>>> switch to using a hashtab that was already done in the kernel and use >>>> that to speed up this checking? Comments? >>> >>> I think that's a good idea. The kernel hashtab and this 'fix' were done >>> about the same time. I intended to bring the kernel hashtab over and >>> got distracted and then forgot about it... >>> >>> Shouldn't be a hard thing, and I believe should get us back to having >>> bitwise the same policy in /sys/fs/selinux/policy as we have on disk... >> >> Looks like we have some other areas of divergence, e.g. range_tr hashtab >> conversion, ebitmap optimizations, possibly others. The latter would >> require a policy binary format change (new version) to make it fully >> identical. >> >> Not sure how well even the hashtab is doing at this point as Fedora has >>> 150,000 file name transition rules in its policy per sedispol output? >> That seems a bit excessive. > > My understanding is that MANY of these are because of the expansion of > attributes in the source and the parent object. If we could leave these > as attributes it would shrink things and incredible amount... I have to question how many of those rules are truly needed. But with regard to enabling attribute usage in file name transition rules, I'd think we'd first need to introduce an avc_transition_sid() interface and cache security_transition_sid() results in the AVC since security_transition_sid() will become more costly. We already do that in the userspace AVC, see avc_compute_create(). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 18:43 ` Eric Paris 2014-04-15 18:46 ` Stephen Smalley @ 2014-04-15 19:07 ` Stephen Smalley 2014-04-15 19:18 ` Eric Paris 1 sibling, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-04-15 19:07 UTC (permalink / raw) To: Eric Paris; +Cc: selinux On 04/15/2014 02:43 PM, Eric Paris wrote: > On Tue, 2014-04-15 at 14:34 -0400, Stephen Smalley wrote: >> On 04/15/2014 02:27 PM, Eric Paris wrote: >>> On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote: >>>> On 04/15/2014 12:40 PM, Stephen Smalley wrote: >>>>> On 04/15/2014 12:27 PM, Richard Haines wrote: >>>>>> The current detection of duplicate rules does not cover the state->out >>>>>> policy and therefore will duplicate filename transition rules if already >>>>>> present. >>>>>> >>>>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> >>>>>> --- >>>>>> libsepol/src/expand.c | 14 ++++++++++++++ >>>>>> 1 file changed, 14 insertions(+) >>>>>> >>>>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c >>>>>> index acb6906..e908fdb 100644 >>>>>> --- a/libsepol/src/expand.c >>>>>> +++ b/libsepol/src/expand.c >>>>>> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r >>>>>> if (cur_trans) >>>>>> continue; >>>>>> >>>>>> + /* Now check if duplicate rule in state->out policy */ >>>>>> + cur_trans = state->out->filename_trans; >>>>>> + >>>>>> + while (cur_trans) { >>>>>> + if (cur_trans->stype == (i + 1) && >>>>>> + cur_trans->ttype == (j + 1) && >>>>>> + cur_trans->tclass == cur_rule->tclass && >>>>>> + !strcmp(cur_trans->name, cur_rule->name)) >>>>>> + break; >>>>>> + cur_trans = cur_trans->next; >>>>>> + } >>>>>> + if (cur_trans) >>>>>> + continue; >>>>>> + >>>>>> new_trans = malloc(sizeof(*new_trans)); >>>>>> if (!new_trans) { >>>>>> ERR(state->handle, "Out of memory!"); >>>>> >>>>> Isn't this effectively a revert of: >>>>> >>>>> commit a29f6820c52b60b9028298cde9962dd140bbf9ea >>>>> Author: Adam Tkac <atkac@redhat.com> >>>>> Date: Fri May 25 17:55:08 2012 +0200 >>>>> >>>>> libsepol: filename_trans: use some better sorting to compare and merge >>>>> >>>>> The kernel switched to using a hashtab for filename_trans rules in >>>>> commit 2463c26d50adc282d19317013ba0ff473823ca47 >>>>> Author: Eric Paris <eparis@redhat.com> >>>>> Date: Thu Apr 28 15:11:21 2011 -0400 >>>>> >>>>> SELinux: put name based create rules in a hashtable >>>>> >>>>> Is there a reason we don't do this in libsepol too? >>>> >>>> So if I am reading a29f68 correctly, it is completely wrong and should >>>> just be reverted. That will fix the duplicate filename transition rules >>>> if I am not mistaken. Then separately, we can look at bringing over the >>>> switch to using a hashtab that was already done in the kernel and use >>>> that to speed up this checking? Comments? >>> >>> I think that's a good idea. The kernel hashtab and this 'fix' were done >>> about the same time. I intended to bring the kernel hashtab over and >>> got distracted and then forgot about it... >>> >>> Shouldn't be a hard thing, and I believe should get us back to having >>> bitwise the same policy in /sys/fs/selinux/policy as we have on disk... >> >> Looks like we have some other areas of divergence, e.g. range_tr hashtab >> conversion, ebitmap optimizations, possibly others. The latter would >> require a policy binary format change (new version) to make it fully >> identical. >> >> Not sure how well even the hashtab is doing at this point as Fedora has >>> 150,000 file name transition rules in its policy per sedispol output? >> That seems a bit excessive. > > My understanding is that MANY of these are because of the expansion of > attributes in the source and the parent object. If we could leave these > as attributes it would shrink things and incredible amount... Hmmm...Fedora policy doesn't build with that change reverted, since it was failing to check all filename transition rules for conflicts: # semodule -B libsepol.expand_filename_trans: Conflicting filename trans rules keyrings staff_gkeyringd_t gnome_home_t : dir otype1:data_home_t otype2:gkeyringd_gnome_home_t libsepol.expand_module: Error during expand libsemanage.semanage_expand_sandbox: Expand module failed semodule: Failed! That's a bug in policy not in libsepol; it was just hidden by the bug in libsepol. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 19:07 ` Stephen Smalley @ 2014-04-15 19:18 ` Eric Paris 2014-04-15 19:19 ` Stephen Smalley 0 siblings, 1 reply; 11+ messages in thread From: Eric Paris @ 2014-04-15 19:18 UTC (permalink / raw) To: Stephen Smalley; +Cc: SE-Linux, Eric Paris Why didn't it fail in the kernel on policy load? On Tue, Apr 15, 2014 at 3:07 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 04/15/2014 02:43 PM, Eric Paris wrote: >> On Tue, 2014-04-15 at 14:34 -0400, Stephen Smalley wrote: >>> On 04/15/2014 02:27 PM, Eric Paris wrote: >>>> On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote: >>>>> On 04/15/2014 12:40 PM, Stephen Smalley wrote: >>>>>> On 04/15/2014 12:27 PM, Richard Haines wrote: >>>>>>> The current detection of duplicate rules does not cover the state->out >>>>>>> policy and therefore will duplicate filename transition rules if already >>>>>>> present. >>>>>>> >>>>>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> >>>>>>> --- >>>>>>> libsepol/src/expand.c | 14 ++++++++++++++ >>>>>>> 1 file changed, 14 insertions(+) >>>>>>> >>>>>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c >>>>>>> index acb6906..e908fdb 100644 >>>>>>> --- a/libsepol/src/expand.c >>>>>>> +++ b/libsepol/src/expand.c >>>>>>> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r >>>>>>> if (cur_trans) >>>>>>> continue; >>>>>>> >>>>>>> + /* Now check if duplicate rule in state->out policy */ >>>>>>> + cur_trans = state->out->filename_trans; >>>>>>> + >>>>>>> + while (cur_trans) { >>>>>>> + if (cur_trans->stype == (i + 1) && >>>>>>> + cur_trans->ttype == (j + 1) && >>>>>>> + cur_trans->tclass == cur_rule->tclass && >>>>>>> + !strcmp(cur_trans->name, cur_rule->name)) >>>>>>> + break; >>>>>>> + cur_trans = cur_trans->next; >>>>>>> + } >>>>>>> + if (cur_trans) >>>>>>> + continue; >>>>>>> + >>>>>>> new_trans = malloc(sizeof(*new_trans)); >>>>>>> if (!new_trans) { >>>>>>> ERR(state->handle, "Out of memory!"); >>>>>> >>>>>> Isn't this effectively a revert of: >>>>>> >>>>>> commit a29f6820c52b60b9028298cde9962dd140bbf9ea >>>>>> Author: Adam Tkac <atkac@redhat.com> >>>>>> Date: Fri May 25 17:55:08 2012 +0200 >>>>>> >>>>>> libsepol: filename_trans: use some better sorting to compare and merge >>>>>> >>>>>> The kernel switched to using a hashtab for filename_trans rules in >>>>>> commit 2463c26d50adc282d19317013ba0ff473823ca47 >>>>>> Author: Eric Paris <eparis@redhat.com> >>>>>> Date: Thu Apr 28 15:11:21 2011 -0400 >>>>>> >>>>>> SELinux: put name based create rules in a hashtable >>>>>> >>>>>> Is there a reason we don't do this in libsepol too? >>>>> >>>>> So if I am reading a29f68 correctly, it is completely wrong and should >>>>> just be reverted. That will fix the duplicate filename transition rules >>>>> if I am not mistaken. Then separately, we can look at bringing over the >>>>> switch to using a hashtab that was already done in the kernel and use >>>>> that to speed up this checking? Comments? >>>> >>>> I think that's a good idea. The kernel hashtab and this 'fix' were done >>>> about the same time. I intended to bring the kernel hashtab over and >>>> got distracted and then forgot about it... >>>> >>>> Shouldn't be a hard thing, and I believe should get us back to having >>>> bitwise the same policy in /sys/fs/selinux/policy as we have on disk... >>> >>> Looks like we have some other areas of divergence, e.g. range_tr hashtab >>> conversion, ebitmap optimizations, possibly others. The latter would >>> require a policy binary format change (new version) to make it fully >>> identical. >>> >>> Not sure how well even the hashtab is doing at this point as Fedora has >>>> 150,000 file name transition rules in its policy per sedispol output? >>> That seems a bit excessive. >> >> My understanding is that MANY of these are because of the expansion of >> attributes in the source and the parent object. If we could leave these >> as attributes it would shrink things and incredible amount... > > Hmmm...Fedora policy doesn't build with that change reverted, since it > was failing to check all filename transition rules for conflicts: > # semodule -B > libsepol.expand_filename_trans: Conflicting filename trans rules > keyrings staff_gkeyringd_t gnome_home_t : dir otype1:data_home_t > otype2:gkeyringd_gnome_home_t > libsepol.expand_module: Error during expand > libsemanage.semanage_expand_sandbox: Expand module failed > semodule: Failed! > > That's a bug in policy not in libsepol; it was just hidden by the bug in > libsepol. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 19:18 ` Eric Paris @ 2014-04-15 19:19 ` Stephen Smalley 2014-04-15 19:37 ` Eric Paris 0 siblings, 1 reply; 11+ messages in thread From: Stephen Smalley @ 2014-04-15 19:19 UTC (permalink / raw) To: Eric Paris; +Cc: SE-Linux, Eric Paris On 04/15/2014 03:18 PM, Eric Paris wrote: > Why didn't it fail in the kernel on policy load? AFAICS you aren't checking for conflicts (and are ignoring duplicates) in your kernel side code that loads the entries. Anyway, the bug is definitely in the policy sources: +filetrans_pattern(gkeyringd_domain, gnome_home_t, gkeyringd_gnome_home_t, dir, "keyrings") +filetrans_pattern(gkeyringd_domain, data_home_t, gkeyringd_gnome_home_t, dir, "keyrings") +filetrans_pattern(gkeyringd_domain, gnome_home_t, data_home_t, dir, "keyrings") > > On Tue, Apr 15, 2014 at 3:07 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 04/15/2014 02:43 PM, Eric Paris wrote: >>> On Tue, 2014-04-15 at 14:34 -0400, Stephen Smalley wrote: >>>> On 04/15/2014 02:27 PM, Eric Paris wrote: >>>>> On Tue, 2014-04-15 at 14:21 -0400, Stephen Smalley wrote: >>>>>> On 04/15/2014 12:40 PM, Stephen Smalley wrote: >>>>>>> On 04/15/2014 12:27 PM, Richard Haines wrote: >>>>>>>> The current detection of duplicate rules does not cover the state->out >>>>>>>> policy and therefore will duplicate filename transition rules if already >>>>>>>> present. >>>>>>>> >>>>>>>> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com> >>>>>>>> --- >>>>>>>> libsepol/src/expand.c | 14 ++++++++++++++ >>>>>>>> 1 file changed, 14 insertions(+) >>>>>>>> >>>>>>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c >>>>>>>> index acb6906..e908fdb 100644 >>>>>>>> --- a/libsepol/src/expand.c >>>>>>>> +++ b/libsepol/src/expand.c >>>>>>>> @@ -1534,6 +1534,20 @@ static int expand_filename_trans(expand_state_t *state, filename_trans_rule_t *r >>>>>>>> if (cur_trans) >>>>>>>> continue; >>>>>>>> >>>>>>>> + /* Now check if duplicate rule in state->out policy */ >>>>>>>> + cur_trans = state->out->filename_trans; >>>>>>>> + >>>>>>>> + while (cur_trans) { >>>>>>>> + if (cur_trans->stype == (i + 1) && >>>>>>>> + cur_trans->ttype == (j + 1) && >>>>>>>> + cur_trans->tclass == cur_rule->tclass && >>>>>>>> + !strcmp(cur_trans->name, cur_rule->name)) >>>>>>>> + break; >>>>>>>> + cur_trans = cur_trans->next; >>>>>>>> + } >>>>>>>> + if (cur_trans) >>>>>>>> + continue; >>>>>>>> + >>>>>>>> new_trans = malloc(sizeof(*new_trans)); >>>>>>>> if (!new_trans) { >>>>>>>> ERR(state->handle, "Out of memory!"); >>>>>>> >>>>>>> Isn't this effectively a revert of: >>>>>>> >>>>>>> commit a29f6820c52b60b9028298cde9962dd140bbf9ea >>>>>>> Author: Adam Tkac <atkac@redhat.com> >>>>>>> Date: Fri May 25 17:55:08 2012 +0200 >>>>>>> >>>>>>> libsepol: filename_trans: use some better sorting to compare and merge >>>>>>> >>>>>>> The kernel switched to using a hashtab for filename_trans rules in >>>>>>> commit 2463c26d50adc282d19317013ba0ff473823ca47 >>>>>>> Author: Eric Paris <eparis@redhat.com> >>>>>>> Date: Thu Apr 28 15:11:21 2011 -0400 >>>>>>> >>>>>>> SELinux: put name based create rules in a hashtable >>>>>>> >>>>>>> Is there a reason we don't do this in libsepol too? >>>>>> >>>>>> So if I am reading a29f68 correctly, it is completely wrong and should >>>>>> just be reverted. That will fix the duplicate filename transition rules >>>>>> if I am not mistaken. Then separately, we can look at bringing over the >>>>>> switch to using a hashtab that was already done in the kernel and use >>>>>> that to speed up this checking? Comments? >>>>> >>>>> I think that's a good idea. The kernel hashtab and this 'fix' were done >>>>> about the same time. I intended to bring the kernel hashtab over and >>>>> got distracted and then forgot about it... >>>>> >>>>> Shouldn't be a hard thing, and I believe should get us back to having >>>>> bitwise the same policy in /sys/fs/selinux/policy as we have on disk... >>>> >>>> Looks like we have some other areas of divergence, e.g. range_tr hashtab >>>> conversion, ebitmap optimizations, possibly others. The latter would >>>> require a policy binary format change (new version) to make it fully >>>> identical. >>>> >>>> Not sure how well even the hashtab is doing at this point as Fedora has >>>>> 150,000 file name transition rules in its policy per sedispol output? >>>> That seems a bit excessive. >>> >>> My understanding is that MANY of these are because of the expansion of >>> attributes in the source and the parent object. If we could leave these >>> as attributes it would shrink things and incredible amount... >> >> Hmmm...Fedora policy doesn't build with that change reverted, since it >> was failing to check all filename transition rules for conflicts: >> # semodule -B >> libsepol.expand_filename_trans: Conflicting filename trans rules >> keyrings staff_gkeyringd_t gnome_home_t : dir otype1:data_home_t >> otype2:gkeyringd_gnome_home_t >> libsepol.expand_module: Error during expand >> libsemanage.semanage_expand_sandbox: Expand module failed >> semodule: Failed! >> >> That's a bug in policy not in libsepol; it was just hidden by the bug in >> libsepol. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy. 2014-04-15 19:19 ` Stephen Smalley @ 2014-04-15 19:37 ` Eric Paris 0 siblings, 0 replies; 11+ messages in thread From: Eric Paris @ 2014-04-15 19:37 UTC (permalink / raw) To: Stephen Smalley; +Cc: SE-Linux On Tue, 2014-04-15 at 15:19 -0400, Stephen Smalley wrote: > On 04/15/2014 03:18 PM, Eric Paris wrote: > > Why didn't it fail in the kernel on policy load? > > AFAICS you aren't checking for conflicts (and are ignoring duplicates) > in your kernel side code that loads the entries. Oh right, that whole 'system wasn't booting' thing. Which pointed out this bug, and then I never fixed it. Perfect. Glad I've been reminded... ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-04-15 19:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-15 16:27 [PATCH] libsepol: Skip duplicate filename_trans rules in state->out policy Richard Haines 2014-04-15 16:40 ` Stephen Smalley 2014-04-15 18:21 ` Stephen Smalley 2014-04-15 18:27 ` Eric Paris 2014-04-15 18:34 ` Stephen Smalley 2014-04-15 18:43 ` Eric Paris 2014-04-15 18:46 ` Stephen Smalley 2014-04-15 19:07 ` Stephen Smalley 2014-04-15 19:18 ` Eric Paris 2014-04-15 19:19 ` Stephen Smalley 2014-04-15 19:37 ` Eric Paris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.