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.
> +
> 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;
> + 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.
