Ill submit a patch for expand_terule_helper() as well, do we want to
retain the assert(0); property on the 2 if/else if/else calsues? Do we
just want to assume that specified is OK since it has never hit the
assert? Do we want to just check specified up front and return an
error:
if (!(specified & AVRULE_TRANSITION|AVRULE_MEMBER|AVRULE_CHANGE)) {
ERR(handle, "Invalid specification: %zu\n", specified);
return EXPAND_RULE_ERROR;
}
On Thu, Nov 17, 2016 at 7:34 AM, William Roberts
<bill.c.roberts@xxxxxxxxx> wrote:
> On Thu, Nov 17, 2016 at 5:36 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
>> On 11/16/2016 04:47 PM, william.c.roberts@xxxxxxxxx wrote:
>>> From: William Roberts <william.c.roberts@xxxxxxxxx>
>>>
>>> General clean up for expand_avrule_helper:
>>> 1. Minimize the conversions of AVRULE specification to AVTAB specification,
>>> they are almost the same, the one exception is AVRULE_DONTAUDIT.
>>> 2. Clean up the if/else logic, collapse with a switch.
>>> 3. Move xperms allocation and manipulation to its own helper.
>>> 4. Only write avkey for values that change.
>>>
>>> Signed-off-by: William Roberts <william.c.roberts@xxxxxxxxx>
>>> ---
>>> libsepol/src/expand.c | 131 +++++++++++++++++++++++++-------------------------
>>> 1 file changed, 66 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
>>> index 3e16f58..fe8a99f 100644
>>> --- a/libsepol/src/expand.c
>>> +++ b/libsepol/src/expand.c
>>> @@ -1781,6 +1781,47 @@ static int expand_terule_helper(sepol_handle_t * handle,
>>> return EXPAND_RULE_SUCCESS;
>>> }
>>>
>>> +/* 0 for success -1 indicates failure */
>>> +static int allocate_xperms(sepol_handle_t * handle, avtab_datum_t * avdatump,
>>> + av_extended_perms_t * extended_perms)
>>> +{
>>> + unsigned int i;
>>> +
>>> + avtab_extended_perms_t *xperms = avdatump->xperms;
>>> + if (!xperms) {
>>> + xperms = (avtab_extended_perms_t *)
>>> + calloc(1, sizeof(avtab_extended_perms_t));
>>> + if (!xperms) {
>>> + ERR(handle, "Out of memory!");
>>> + return -1;
>>> + }
>>> + avdatump->xperms = xperms;
>>> + }
>>> +
>>> + switch (extended_perms->specified) {
>>> + case AVRULE_XPERMS_IOCTLFUNCTION:
>>> + xperms->specified = AVTAB_XPERMS_IOCTLFUNCTION;
>>> + break;
>>> + case AVRULE_XPERMS_IOCTLDRIVER:
>>> + xperms->specified = AVTAB_XPERMS_IOCTLDRIVER;
>>> + break;
>>> + default:
>>> + return -1;
>>> + }
>>> +
>>> + xperms->driver = extended_perms->driver;
>>> + for (i = 0; i < ARRAY_SIZE(xperms->perms); i++)
>>> + xperms->perms[i] |= extended_perms->perms[i];
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static uint32_t avrule_to_avtab_spec(uint32_t specification)
>>> +{
>>> + return (specification == AVRULE_DONTAUDIT) ?
>>> + AVTAB_AUDITDENY : specification;
>>> +}
>>
>> Doesn't seem to merit its own helper function since it is only ever used
>> once and is a one-liner.
>
> It should be usable in: expand_terule_helper()
>
>>
>>> +
>>> static int expand_avrule_helper(sepol_handle_t * handle,
>>> uint32_t specified,
>>> cond_av_list_t ** cond,
>>> @@ -1790,44 +1831,21 @@ static int expand_avrule_helper(sepol_handle_t * handle,
>>> {
>>> avtab_key_t avkey;
>>> avtab_datum_t *avdatump;
>>> - avtab_extended_perms_t *xperms;
>>> avtab_ptr_t node;
>>> class_perm_node_t *cur;
>>> - uint32_t spec = 0;
>>> - unsigned int i;
>>>
>>> - if (specified & AVRULE_ALLOWED) {
>>> - spec = AVTAB_ALLOWED;
>>> - } else if (specified & AVRULE_AUDITALLOW) {
>>> - spec = AVTAB_AUDITALLOW;
>>> - } else if (specified & AVRULE_AUDITDENY) {
>>> - spec = AVTAB_AUDITDENY;
>>> - } else if (specified & AVRULE_DONTAUDIT) {
>>> - if (handle && handle->disable_dontaudit)
>>> - return EXPAND_RULE_SUCCESS;
>>> - spec = AVTAB_AUDITDENY;
>>> - } else if (specified & AVRULE_NEVERALLOW) {
>>> - spec = AVTAB_NEVERALLOW;
>>> - } else if (specified & AVRULE_XPERMS_ALLOWED) {
>>> - spec = AVTAB_XPERMS_ALLOWED;
>>> - } else if (specified & AVRULE_XPERMS_AUDITALLOW) {
>>> - spec = AVTAB_XPERMS_AUDITALLOW;
>>> - } else if (specified & AVRULE_XPERMS_DONTAUDIT) {
>>> - if (handle && handle->disable_dontaudit)
>>> + /* bail early if dontaudit's are disabled and it's a dontaudit rule */
>>> + if ((specified & (AVRULE_DONTAUDIT|AVRULE_XPERMS_DONTAUDIT))
>>> + && handle && handle->disable_dontaudit)
>>> return EXPAND_RULE_SUCCESS;
>>> - spec = AVTAB_XPERMS_DONTAUDIT;
>>> - } else if (specified & AVRULE_XPERMS_NEVERALLOW) {
>>> - spec = AVTAB_XPERMS_NEVERALLOW;
>>> - } else {
>>> - assert(0); /* unreachable */
>>> - }
>>> +
>>> + avkey.source_type = stype + 1;
>>> + avkey.target_type = ttype + 1;
>>> + avkey.specified = avrule_to_avtab_spec(specified);
>>>
>>> cur = perms;
>>> while (cur) {
>>> - avkey.source_type = stype + 1;
>>> - avkey.target_type = ttype + 1;
>>> avkey.target_class = cur->tclass;
>>> - avkey.specified = spec;
>>>
>>> node = find_avtab_node(handle, avtab, &avkey, cond, extended_perms);
>>> if (!node)
>>> @@ -1839,13 +1857,15 @@ static int expand_avrule_helper(sepol_handle_t * handle,
>>> }
>>>
>>> avdatump = &node->datum;
>>> - if (specified & AVRULE_ALLOWED) {
>>> - avdatump->data |= cur->data;
>>> - } else if (specified & AVRULE_AUDITALLOW) {
>>> - avdatump->data |= cur->data;
>>> - } else if (specified & AVRULE_NEVERALLOW) {
>>> + switch (specified) {
>>> + case AVRULE_ALLOWED:
>>> + case AVRULE_AUDITALLOW:
>>> + case AVRULE_NEVERALLOW:
>>> avdatump->data |= cur->data;
>>> - } else if (specified & AVRULE_AUDITDENY) {
>>> + break;
>>> + case AVRULE_DONTAUDIT:
>>> + cur->data = ~cur->data;
>>
>> Would prefer to keep them separate, and to not mutate cur at all, i.e.
>> avdatump->data &= ~cur->data;
>> break;
>
> Fine.
>
>>
>>> + case AVRULE_AUDITDENY:
>>> /* Since a '0' in an auditdeny mask represents
>>> * a permission we do NOT want to audit
>>> * (dontaudit), we use the '&' operand to
>>> @@ -1854,35 +1874,16 @@ static int expand_avrule_helper(sepol_handle_t * handle,
>>> * auditallow cases).
>>> */
>>> avdatump->data &= cur->data;
>>> - } else if (specified & AVRULE_DONTAUDIT) {
>>> - avdatump->data &= ~cur->data;
>>> - } else if (specified & AVRULE_XPERMS) {
>>> - xperms = avdatump->xperms;
>>> - if (!xperms) {
>>> - xperms = (avtab_extended_perms_t *)
>>> - calloc(1, sizeof(avtab_extended_perms_t));
>>> - if (!xperms) {
>>> - ERR(handle, "Out of memory!");
>>> - return -1;
>>> - }
>>> - avdatump->xperms = xperms;
>>> - }
>>> -
>>> - switch (extended_perms->specified) {
>>> - case AVRULE_XPERMS_IOCTLFUNCTION:
>>> - xperms->specified = AVTAB_XPERMS_IOCTLFUNCTION;
>>> - break;
>>> - case AVRULE_XPERMS_IOCTLDRIVER:
>>> - xperms->specified = AVTAB_XPERMS_IOCTLDRIVER;
>>> - break;
>>> - default:
>>> - return -1;
>>> - }
>>> -
>>> - xperms->driver = extended_perms->driver;
>>> - for (i = 0; i < ARRAY_SIZE(xperms->perms); i++)
>>> - xperms->perms[i] |= extended_perms->perms[i];
>>> - } else {
>>> + break;
>>> + case AVRULE_XPERMS_ALLOWED:
>>> + case AVRULE_XPERMS_AUDITALLOW:
>>> + case AVRULE_XPERMS_DONTAUDIT:
>>> + case AVRULE_XPERMS_NEVERALLOW:
>>> + if (allocate_xperms(handle, avdatump, extended_perms))
>>> + return EXPAND_RULE_ERROR;
>>> + break;
>>> + default:
>>> + ERR(handle, "Unknown specification: %x\n", specified);
>>> assert(0); /* should never occur */
>>> }
>>>
>>>
>>
>> _______________________________________________
>> 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
--
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.
