On 11/17/2016 10:46 AM, William Roberts wrote:
> 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;
> }
Doing a runtime check as you suggest above is fine (except you need
different parenthesization).
>
> 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
>
>
>
_______________________________________________
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.
