* [PATCH] libsepol: Eliminate gaps in the policydb role arrays
@ 2021-02-05 20:49 James Carter
2021-02-18 7:25 ` Nicolas Iooss
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2021-02-05 20:49 UTC (permalink / raw)
To: selinux; +Cc: James Carter
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];
+ }
buf[1] = cpu_to_le32(buf[1]);
items = put_entry(buf, sizeof(uint32_t), 2, fp);
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libsepol: Eliminate gaps in the policydb role arrays
2021-02-05 20:49 [PATCH] libsepol: Eliminate gaps in the policydb role arrays James Carter
@ 2021-02-18 7:25 ` Nicolas Iooss
2021-02-18 15:05 ` James Carter
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Iooss @ 2021-02-18 7:25 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libsepol: Eliminate gaps in the policydb role arrays
2021-02-18 7:25 ` Nicolas Iooss
@ 2021-02-18 15:05 ` James Carter
0 siblings, 0 replies; 3+ messages in thread
From: James Carter @ 2021-02-18 15:05 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-02-18 17:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 20:49 [PATCH] libsepol: Eliminate gaps in the policydb role arrays James Carter
2021-02-18 7:25 ` Nicolas Iooss
2021-02-18 15:05 ` James Carter
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.