Re: [PATCH take2 v2] libsepol: fix checkpolicy dontaudit compiler bug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 15, 2016 at 1:17 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 11/15/2016 04:06 PM, william.c.roberts@xxxxxxxxx wrote:
>> From: William Roberts <william.c.roberts@xxxxxxxxx>
>>
>> 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/6201bb5e2/commit 6201bb5e258e2b5bcc04d502d6fbc05c69d21d71 ("libsepol:
> fix checkpolicy dontaudit compiler bug")/
>
>>
>> Reported-by: Nick Kralevich <nnk@xxxxxxxxxx>
>> Signed-off-by: William Roberts <william.c.roberts@xxxxxxxxx>
>> ---
>>  libsepol/src/expand.c | 27 ++++++++++++++++++++-------
>>  1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
>> index 004a029..815d012 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) {
>> @@ -1663,6 +1665,17 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>>       return node;
>>  }
>>
>> +static inline uint32_t specified_to_init_value(uint32_t specified)
>> +{
>> +     /*
>> +      * Things that get &= start life with values as ~0 and get unset as
>> +      * they continue life.
>> +      * Things that are |=, start as 0.
>> +      */
>> +     return (specified & AVRULE_DONTAUDIT)
>> +             || (specified & AVRULE_AUDITDENY) ? ~0 : 0;
>
> This can be simpler, e.g. (specified &
> (AVRULE_DONTAUDIT|AVRULE_AUDITDENY)).

True.

 In fact, you can do one better by
> passing spec rather than specified in which case it is just (spec ==
> AVTAB_AUDITDENY) since they are both converted to auditdeny vectors in
> the avtab.

I didn't want to pollute the subroutine with knowledge of spec, but
considering this is all
in one C file, I suppose it doesn't matter. I actually had that originally.

>
>> +}
>> +
>>  #define EXPAND_RULE_SUCCESS   1
>>  #define EXPAND_RULE_CONFLICT  0
>>  #define EXPAND_RULE_ERROR    -1
>> @@ -1750,7 +1763,8 @@ 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,
>> +                                    specified_to_init_value(specified));
>
> This one should always be 0, since the datum is a type value, not an
> access vector.

Roger that.

>
>>               if (!node)
>>                       return -1;
>>               if (enabled) {
>> @@ -1824,7 +1838,9 @@ 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,
>> +                                    specified_to_init_value(specified));
>>               if (!node)
>>                       return EXPAND_RULE_ERROR;
>>               if (enabled) {
>> @@ -1850,10 +1866,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@xxxxxxxxxxxxx
> To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
> To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.



-- 
Respectfully,

William C Roberts
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.



[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux