| Message ID | 20210205204934.314141-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libsepol: Eliminate gaps in the policydb role arrays | expand |
On Fri, Feb 5, 2021 at 9:51 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > --- > 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
On Thu, Feb 18, 2021 at 2:25 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > On Fri, Feb 5, 2021 at 9:51 PM James Carter <jwcart2@gmail.com> 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@gmail.com> > > --- > > 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); > You are correct. I also noticed with more testing that I can't update buf[0] when writing modules. I've also found some other problems when trying to create modular policy with a version before role attributes if role attributes exist. I am trying to figure out the best way to handle the situation. Thanks for the review. Jim > Thanks! > Nicolas >
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]; + } buf[1] = cpu_to_le32(buf[1]); items = put_entry(buf, sizeof(uint32_t), 2, fp);
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@gmail.com> --- libsepol/src/expand.c | 40 ++++++++++++++++++++++++++++++++-------- libsepol/src/write.c | 6 ++++-- 2 files changed, 36 insertions(+), 10 deletions(-)