expand_avrule_helper() does ERR() assert, I would imagine we would
want them to be consistent, so do you want me to change that to ERR()
return, or change expand_te_rule_helper() to ERR() assert(0)?
On Thu, Nov 17, 2016 at 7:52 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> 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
>>
>>
>>
>
--
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.
