* [PATCH v2 1/2] policydb.h: use AVTAB macros to avoid duplications
@ 2016-11-16 21:47 william.c.roberts
2016-11-16 21:47 ` [PATCH v2 2/2] expand_avrule_helper: cleanup william.c.roberts
0 siblings, 1 reply; 10+ messages in thread
From: william.c.roberts @ 2016-11-16 21:47 UTC (permalink / raw)
To: sds, selinux
From: William Roberts <william.c.roberts@intel.com>
Rather than having multiple copies of the AVTAB and AVRULE
defines, consolidate them.
This makes it clear that AVRULE to AVTAB conversion no longer
need to occur.
Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
libsepol/include/sepol/policydb/policydb.h | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
index d99fcf4..77e46fb 100644
--- a/libsepol/include/sepol/policydb/policydb.h
+++ b/libsepol/include/sepol/policydb/policydb.h
@@ -258,20 +258,20 @@ typedef struct av_extended_perms {
typedef struct avrule {
/* these typedefs are almost exactly the same as those in avtab.h - they are
* here because of the need to include neverallow and dontaudit messages */
-#define AVRULE_ALLOWED 0x0001
-#define AVRULE_AUDITALLOW 0x0002
-#define AVRULE_AUDITDENY 0x0004
+#define AVRULE_ALLOWED AVTAB_ALLOWED
+#define AVRULE_AUDITALLOW AVTAB_AUDITALLOW
+#define AVRULE_AUDITDENY AVTAB_AUDITDENY
#define AVRULE_DONTAUDIT 0x0008
-#define AVRULE_NEVERALLOW 0x0080
+#define AVRULE_NEVERALLOW AVTAB_NEVERALLOW
#define AVRULE_AV (AVRULE_ALLOWED | AVRULE_AUDITALLOW | AVRULE_AUDITDENY | AVRULE_DONTAUDIT | AVRULE_NEVERALLOW)
-#define AVRULE_TRANSITION 0x0010
-#define AVRULE_MEMBER 0x0020
-#define AVRULE_CHANGE 0x0040
+#define AVRULE_TRANSITION AVTAB_TRANSITION
+#define AVRULE_MEMBER AVTAB_MEMBER
+#define AVRULE_CHANGE AVTAB_CHANGE
#define AVRULE_TYPE (AVRULE_TRANSITION | AVRULE_MEMBER | AVRULE_CHANGE)
-#define AVRULE_XPERMS_ALLOWED 0x0100
-#define AVRULE_XPERMS_AUDITALLOW 0x0200
-#define AVRULE_XPERMS_DONTAUDIT 0x0400
-#define AVRULE_XPERMS_NEVERALLOW 0x0800
+#define AVRULE_XPERMS_ALLOWED AVTAB_XPERMS_ALLOWED
+#define AVRULE_XPERMS_AUDITALLOW AVTAB_XPERMS_AUDITALLOW
+#define AVRULE_XPERMS_DONTAUDIT AVTAB_XPERMS_DONTAUDIT
+#define AVRULE_XPERMS_NEVERALLOW AVTAB_XPERMS_NEVERALLOW
#define AVRULE_XPERMS (AVRULE_XPERMS_ALLOWED | AVRULE_XPERMS_AUDITALLOW | \
AVRULE_XPERMS_DONTAUDIT | AVRULE_XPERMS_NEVERALLOW)
uint32_t specified;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-16 21:47 [PATCH v2 1/2] policydb.h: use AVTAB macros to avoid duplications william.c.roberts
@ 2016-11-16 21:47 ` william.c.roberts
2016-11-17 13:36 ` Stephen Smalley
2016-11-17 16:55 ` William Roberts
0 siblings, 2 replies; 10+ messages in thread
From: william.c.roberts @ 2016-11-16 21:47 UTC (permalink / raw)
To: sds, selinux
From: William Roberts <william.c.roberts@intel.com>
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@intel.com>
---
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;
+}
+
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;
+ 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 */
}
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-16 21:47 ` [PATCH v2 2/2] expand_avrule_helper: cleanup william.c.roberts
@ 2016-11-17 13:36 ` Stephen Smalley
2016-11-17 15:34 ` William Roberts
2016-11-17 16:55 ` William Roberts
1 sibling, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2016-11-17 13:36 UTC (permalink / raw)
To: william.c.roberts, selinux
On 11/16/2016 04:47 PM, william.c.roberts@intel.com wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> 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@intel.com>
> ---
> 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 */
> }
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-17 13:36 ` Stephen Smalley
@ 2016-11-17 15:34 ` William Roberts
2016-11-17 15:46 ` William Roberts
2016-11-17 15:50 ` Stephen Smalley
0 siblings, 2 replies; 10+ messages in thread
From: William Roberts @ 2016-11-17 15:34 UTC (permalink / raw)
To: Stephen Smalley; +Cc: William Roberts, selinux
On Thu, Nov 17, 2016 at 5:36 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 11/16/2016 04:47 PM, william.c.roberts@intel.com wrote:
>> From: William Roberts <william.c.roberts@intel.com>
>>
>> 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@intel.com>
>> ---
>> 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@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
--
Respectfully,
William C Roberts
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-17 15:34 ` William Roberts
@ 2016-11-17 15:46 ` William Roberts
2016-11-17 15:52 ` Stephen Smalley
2016-11-17 15:50 ` Stephen Smalley
1 sibling, 1 reply; 10+ messages in thread
From: William Roberts @ 2016-11-17 15:46 UTC (permalink / raw)
To: Stephen Smalley; +Cc: William Roberts, selinux
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@gmail.com> wrote:
> On Thu, Nov 17, 2016 at 5:36 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 11/16/2016 04:47 PM, william.c.roberts@intel.com wrote:
>>> From: William Roberts <william.c.roberts@intel.com>
>>>
>>> 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@intel.com>
>>> ---
>>> 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@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>
>
>
> --
> Respectfully,
>
> William C Roberts
--
Respectfully,
William C Roberts
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-17 15:34 ` William Roberts
2016-11-17 15:46 ` William Roberts
@ 2016-11-17 15:50 ` Stephen Smalley
1 sibling, 0 replies; 10+ messages in thread
From: Stephen Smalley @ 2016-11-17 15:50 UTC (permalink / raw)
To: William Roberts; +Cc: William Roberts, selinux
On 11/17/2016 10:34 AM, William Roberts wrote:
> On Thu, Nov 17, 2016 at 5:36 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 11/16/2016 04:47 PM, william.c.roberts@intel.com wrote:
>>> From: William Roberts <william.c.roberts@intel.com>
>>>
>>> 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@intel.com>
>>> ---
>>> 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()
No mapping required there.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-17 15:52 ` Stephen Smalley
@ 2016-11-17 15:52 ` William Roberts
2016-11-17 15:56 ` Stephen Smalley
0 siblings, 1 reply; 10+ messages in thread
From: William Roberts @ 2016-11-17 15:52 UTC (permalink / raw)
To: Stephen Smalley; +Cc: William Roberts, selinux
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@tycho.nsa.gov> 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@gmail.com> wrote:
>>> On Thu, Nov 17, 2016 at 5:36 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>> On 11/16/2016 04:47 PM, william.c.roberts@intel.com wrote:
>>>>> From: William Roberts <william.c.roberts@intel.com>
>>>>>
>>>>> 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@intel.com>
>>>>> ---
>>>>> 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@tycho.nsa.gov
>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>>
>>>
>>>
>>> --
>>> Respectfully,
>>>
>>> William C Roberts
>>
>>
>>
>
--
Respectfully,
William C Roberts
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-17 15:46 ` William Roberts
@ 2016-11-17 15:52 ` Stephen Smalley
2016-11-17 15:52 ` William Roberts
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2016-11-17 15:52 UTC (permalink / raw)
To: William Roberts; +Cc: William Roberts, selinux
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@gmail.com> wrote:
>> On Thu, Nov 17, 2016 at 5:36 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On 11/16/2016 04:47 PM, william.c.roberts@intel.com wrote:
>>>> From: William Roberts <william.c.roberts@intel.com>
>>>>
>>>> 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@intel.com>
>>>> ---
>>>> 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@tycho.nsa.gov
>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>
>>
>>
>> --
>> Respectfully,
>>
>> William C Roberts
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-17 15:52 ` William Roberts
@ 2016-11-17 15:56 ` Stephen Smalley
0 siblings, 0 replies; 10+ messages in thread
From: Stephen Smalley @ 2016-11-17 15:56 UTC (permalink / raw)
To: William Roberts; +Cc: William Roberts, selinux
On 11/17/2016 10:52 AM, William Roberts wrote:
> 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)?
I'd make it ERR() return
>
> On Thu, Nov 17, 2016 at 7:52 AM, Stephen Smalley <sds@tycho.nsa.gov> 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@gmail.com> wrote:
>>>> On Thu, Nov 17, 2016 at 5:36 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>>>> On 11/16/2016 04:47 PM, william.c.roberts@intel.com wrote:
>>>>>> From: William Roberts <william.c.roberts@intel.com>
>>>>>>
>>>>>> 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@intel.com>
>>>>>> ---
>>>>>> 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@tycho.nsa.gov
>>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>>>
>>>>
>>>>
>>>> --
>>>> Respectfully,
>>>>
>>>> William C Roberts
>>>
>>>
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] expand_avrule_helper: cleanup
2016-11-16 21:47 ` [PATCH v2 2/2] expand_avrule_helper: cleanup william.c.roberts
2016-11-17 13:36 ` Stephen Smalley
@ 2016-11-17 16:55 ` William Roberts
1 sibling, 0 replies; 10+ messages in thread
From: William Roberts @ 2016-11-17 16:55 UTC (permalink / raw)
To: William Roberts; +Cc: Stephen Smalley, selinux
On Wed, Nov 16, 2016 at 1:47 PM, <william.c.roberts@intel.com> wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> 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@intel.com>
> ---
> 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:
I forced this path to make it bail on the allocation. I didn't see it pull up in
the leak report from valgrind. But in general, checkpolicy leaks, but nothing
allocated in expand.c.
> + 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;
> +}
> +
> 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;
> + 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 */
> }
>
> --
> 2.7.4
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
--
Respectfully,
William C Roberts
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-11-17 16:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-16 21:47 [PATCH v2 1/2] policydb.h: use AVTAB macros to avoid duplications william.c.roberts
2016-11-16 21:47 ` [PATCH v2 2/2] expand_avrule_helper: cleanup william.c.roberts
2016-11-17 13:36 ` Stephen Smalley
2016-11-17 15:34 ` William Roberts
2016-11-17 15:46 ` William Roberts
2016-11-17 15:52 ` Stephen Smalley
2016-11-17 15:52 ` William Roberts
2016-11-17 15:56 ` Stephen Smalley
2016-11-17 15:50 ` Stephen Smalley
2016-11-17 16:55 ` William Roberts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.