On Thu, Feb 18, 2021 at 2:25 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
>
> 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);
>
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
>
