On Wed, Nov 16, 2016 at 5:59 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 11/15/2016 06:37 PM, William Roberts wrote:
>> On Tue, Nov 15, 2016 at 3:21 PM, William Roberts
>> <bill.c.roberts@xxxxxxxxx> wrote:
>>> On Tue, Nov 15, 2016 at 1:53 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
>>>> On 11/15/2016 04:42 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 reimplementation of:
>>>>>
>>>>> /commit 6201bb5e258e2b5bcc04d502d6fbc05c69d21d71 ("libsepol:
>>>>> fix checkpolicy dontaudit compiler bug")
>>>>
>>>> extraneous / and whitespace
>>>>
>>>>>
>>>>> that avoids the cumbersome pointer assignments on alloced.
>>>>>
>>>>> Reported-by: Nick Kralevich <nnk@xxxxxxxxxx>
>>>>> Signed-off-by: William Roberts <william.c.roberts@xxxxxxxxx>
>>>>> ---
>>>>> libsepol/src/expand.c | 18 +++++++++++-------
>>>>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
>>>>> index 004a029..78905d9 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 spec)
>>>>
>>>> Sorry, it occurred to me belatedly that you already have the spec value
>>>> via key->specified (it is the avtab value, so it is the right one). No
>>>> need for an additional argument.
>>>
>>> That's ideal, I saw its usage for XPERMS, but it's unclear why spec,
>>> key->specified and
>>> specified all exist within those call paths, seems clunky to me.
>>>
>>> It's likely not normalized so will need to bitwise and out the
>>> DONTAUDIT and AUDITDENY
>>> masks for the initialization value branch.
>>
>> So its assigned to the normalized spec around line 1831:
>> avkey.specified = spec;
>>
>> This means, couldn't the if/else nightmare below go to a switch, so
>> then the |= and &=
>> just share a case?
>
> Probably for |=. With AUDITDENY vs DONTAUDIT there is the difference in
> whether we use cur->data or ~cur->data.
>
>> Also the spec intermediary could go away with a little massaging. Why
>> does this need
>> to be normalized, is their a case were the passed in specified has
>> more than one bit set?
>
> Well, we do need to convert from AVRULE_* to AVTAB_* regardless, and
> there is no AVTAB_DONTAUDIT, only AVTAB_AUDITDENY. Doesn't appear that
> there can ever be more than one bit set anymore; originally a single
> avtab entry could store all of the allow/auditallow/auditdeny/transition
> values for a given (source type, target type, target class) triple, but
> I split them out long ago as part of optimizing the avtab memory usage.
>
Sweet. I have a patch coming that trims the fat out of this function.
_______________________________________________
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.
