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
_______________________________________________
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.
