On Fri, Feb 5, 2021 at 9:51 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> Since the kernel binary policy does not include role attributes,
> they are expanded when creating the policy and there are gaps
> in the role values written to the policy. When this policy is
> read from a file and the policydb is created there will be gaps
> in the p_role_val_to_name and role_val_to_struct arrays.
>
> When expanding a policy into a new policydb, copy the roles first
> and then copy the role attributes. When writing a kernel binary
> policy, update the nprim of the role symtab by subtracting the number
> of role attributes. Now when that policy is read and its policydb is
> created there will be no gaps in the role arrays.
>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> libsepol/src/expand.c | 40 ++++++++++++++++++++++++++++++++--------
> libsepol/src/write.c | 6 ++++--
> 2 files changed, 36 insertions(+), 10 deletions(-)
>
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index eac7e450..ea1b115a 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -789,19 +789,15 @@ static int role_fix_callback(hashtab_key_t key, hashtab_datum_t datum,
> return 0;
> }
>
> -static int role_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
> - void *data)
> +static int role_copy_callback_helper(char *id, role_datum_t *role, expand_state_t *state, unsigned int copy_attr)
> {
> int ret;
> - char *id, *new_id;
> - role_datum_t *role;
> + char *new_id;
> role_datum_t *new_role;
> - expand_state_t *state;
> ebitmap_t tmp_union_types;
>
> - id = key;
> - role = (role_datum_t *) datum;
> - state = (expand_state_t *) data;
> + if ((!copy_attr && role->flavor == ROLE_ATTRIB) || (copy_attr && role->flavor != ROLE_ATTRIB))
> + return 0;
>
> if (strcmp(id, OBJECT_R) == 0) {
> /* object_r is always value 1 */
> @@ -878,6 +874,26 @@ static int role_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
> return 0;
> }
>
> +static int role_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
> + void *data)
> +{
> + char *id = key;
> + role_datum_t *role = (role_datum_t *) datum;
> + expand_state_t *state = (expand_state_t *) data;
> +
> + return role_copy_callback_helper(id, role, state, 0);
> +}
> +
> +static int role_attr_copy_callback(hashtab_key_t key, hashtab_datum_t datum,
> + void *data)
> +{
> + char *id = key;
> + role_datum_t *role = (role_datum_t *) datum;
> + expand_state_t *state = (expand_state_t *) data;
> +
> + return role_copy_callback_helper(id, role, state, 1);
> +}
> +
> int mls_semantic_level_expand(mls_semantic_level_t * sl, mls_level_t * l,
> policydb_t * p, sepol_handle_t * h)
> {
> @@ -3014,6 +3030,9 @@ int expand_module(sepol_handle_t * handle,
> /* copy roles */
> if (hashtab_map(state.base->p_roles.table, role_copy_callback, &state))
> goto cleanup;
> + /* copy role attrs */
> + if (hashtab_map(state.base->p_roles.table, role_attr_copy_callback, &state))
> + goto cleanup;
> if (hashtab_map(state.base->p_roles.table,
> role_bounds_copy_callback, &state))
> goto cleanup;
> @@ -3074,6 +3093,11 @@ int expand_module(sepol_handle_t * handle,
> (decl->p_roles.table, role_copy_callback, &state))
> goto cleanup;
>
> + /* copy role attrs */
> + if (hashtab_map
> + (decl->p_roles.table, role_attr_copy_callback, &state))
> + goto cleanup;
> +
> /* copy users */
> if (hashtab_map
> (decl->p_users.table, user_copy_callback, &state))
> diff --git a/libsepol/src/write.c b/libsepol/src/write.c
> index 84bcaf3f..95a2c376 100644
> --- a/libsepol/src/write.c
> +++ b/libsepol/src/write.c
> @@ -2321,8 +2321,10 @@ int policydb_write(policydb_t * p, struct policy_file *fp)
> if ((i == SYM_ROLES) &&
> ((p->policy_type == POLICY_KERN) ||
> (p->policy_type != POLICY_KERN &&
> - p->policyvers < MOD_POLICYDB_VERSION_ROLEATTRIB)))
> - (void)hashtab_map(p->symtab[i].table, role_attr_uncount, &buf[1]);
> + p->policyvers < MOD_POLICYDB_VERSION_ROLEATTRIB))) {
> + hashtab_map(p->symtab[i].table, role_attr_uncount, &buf[1]);
> + buf[0] -= p->symtab[i].table->nel - buf[1];
Hello,
Sorry for the delay, I was busy in the last few days.
While reviewing this patch, I stumbled upon this line which changes
buf[0]. At the beginning of the for where these lines are modified
there is:
buf[0] = cpu_to_le32(p->symtab[i].nprim);
Does doing "buf[0] -= ..." works on Big Endian systems? It would be
more intuitive if the code was:
buf[0] = p->symtab[i].nprim
/* ... */
if (...) {
buf[0] -= p->symtab[i].table->nel - buf[1];
}
buf[0] = cpu_to_le32(buf[0]);
buf[1] = cpu_to_le32(buf[1]);
items = put_entry(buf, sizeof(uint32_t), 2, fp);
Thanks!
Nicolas
