On Wed, Oct 18, 2023 at 1:58 PM Jacob Satterfield
<jsatterfield.linux@xxxxxxxxx> wrote:
>
> In four separate functions within avtab, the same comparison logic is
> used. The only difference is how the result is handled or whether there
> is a unique specifier value to be checked for or used.
>
> Extracting this functionality into the avtab_node_cmp() function unifies
> the comparison logic between searching and insertion and gets rid of
> duplicative code so that the implementation is easier to maintain.
>
> Signed-off-by: Jacob Satterfield <jsatterfield.linux@xxxxxxxxx>
> Reviewed-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
Nit: You can't retain a Reviewed-by line if you make any changes to
the previously reviewed patch.
That said, having re-reviewed this one, you can add my Reviewed-by tag
to your next one if there are no changes.
> ---
> security/selinux/ss/avtab.c | 101 +++++++++++++++---------------------
> 1 file changed, 41 insertions(+), 60 deletions(-)
>
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 8751a602ead2..697eb4352439 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -96,12 +96,34 @@ avtab_insert_node(struct avtab *h, struct avtab_node **dst,
> return newnode;
> }
>
> +static int avtab_node_cmp(const struct avtab_key *key1,
> + const struct avtab_key *key2)
> +{
> + u16 specified = key1->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
> +
> + if (key1->source_type == key2->source_type &&
> + key1->target_type == key2->target_type &&
> + key1->target_class == key2->target_class &&
> + (specified & key2->specified))
> + return 0;
> + if (key1->source_type < key2->source_type)
> + return -1;
> + if (key1->source_type == key2->source_type &&
> + key1->target_type < key2->target_type)
> + return -1;
> + if (key1->source_type == key2->source_type &&
> + key1->target_type == key2->target_type &&
> + key1->target_class < key2->target_class)
> + return -1;
> + return 1;
> +}
> +
> static int avtab_insert(struct avtab *h, const struct avtab_key *key,
> const struct avtab_datum *datum)
> {
> u32 hvalue;
> struct avtab_node *prev, *cur, *newnode;
> - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
> + int cmp;
>
> if (!h || !h->nslot || h->nel == U32_MAX)
> return -EINVAL;
> @@ -110,23 +132,11 @@ static int avtab_insert(struct avtab *h, const struct avtab_key *key,
> for (prev = NULL, cur = h->htable[hvalue];
> cur;
> prev = cur, cur = cur->next) {
> - if (key->source_type == cur->key.source_type &&
> - key->target_type == cur->key.target_type &&
> - key->target_class == cur->key.target_class &&
> - (specified & cur->key.specified)) {
> - /* extended perms may not be unique */
> - if (specified & AVTAB_XPERMS)
> - break;
> + cmp = avtab_node_cmp(key, &cur->key);
> + /* extended perms may not be unique */
> + if (cmp == 0 && !(key->specified & AVTAB_XPERMS))
> return -EEXIST;
> - }
> - if (key->source_type < cur->key.source_type)
> - break;
> - if (key->source_type == cur->key.source_type &&
> - key->target_type < cur->key.target_type)
> - break;
> - if (key->source_type == cur->key.source_type &&
> - key->target_type == cur->key.target_type &&
> - key->target_class < cur->key.target_class)
> + if (cmp <= 0)
> break;
> }
>
> @@ -148,7 +158,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h,
> {
> u32 hvalue;
> struct avtab_node *prev, *cur;
> - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
> + int cmp;
>
> if (!h || !h->nslot || h->nel == U32_MAX)
> return NULL;
> @@ -156,19 +166,8 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h,
> for (prev = NULL, cur = h->htable[hvalue];
> cur;
> prev = cur, cur = cur->next) {
> - if (key->source_type == cur->key.source_type &&
> - key->target_type == cur->key.target_type &&
> - key->target_class == cur->key.target_class &&
> - (specified & cur->key.specified))
> - break;
> - if (key->source_type < cur->key.source_type)
> - break;
> - if (key->source_type == cur->key.source_type &&
> - key->target_type < cur->key.target_type)
> - break;
> - if (key->source_type == cur->key.source_type &&
> - key->target_type == cur->key.target_type &&
> - key->target_class < cur->key.target_class)
> + cmp = avtab_node_cmp(key, &cur->key);
> + if (cmp <= 0)
> break;
> }
> return avtab_insert_node(h, prev ? &prev->next : &h->htable[hvalue],
> @@ -183,7 +182,7 @@ struct avtab_node *avtab_search_node(struct avtab *h,
> {
> u32 hvalue;
> struct avtab_node *cur;
> - u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
> + int cmp;
>
> if (!h || !h->nslot)
> return NULL;
> @@ -191,20 +190,10 @@ struct avtab_node *avtab_search_node(struct avtab *h,
> hvalue = avtab_hash(key, h->mask);
> for (cur = h->htable[hvalue]; cur;
> cur = cur->next) {
> - if (key->source_type == cur->key.source_type &&
> - key->target_type == cur->key.target_type &&
> - key->target_class == cur->key.target_class &&
> - (specified & cur->key.specified))
> + cmp = avtab_node_cmp(key, &cur->key);
> + if (cmp == 0)
> return cur;
> -
> - if (key->source_type < cur->key.source_type)
> - break;
> - if (key->source_type == cur->key.source_type &&
> - key->target_type < cur->key.target_type)
> - break;
> - if (key->source_type == cur->key.source_type &&
> - key->target_type == cur->key.target_type &&
> - key->target_class < cur->key.target_class)
> + if (cmp < 0)
> break;
> }
> return NULL;
> @@ -213,27 +202,19 @@ struct avtab_node *avtab_search_node(struct avtab *h,
> struct avtab_node*
> avtab_search_node_next(struct avtab_node *node, u16 specified)
> {
> + struct avtab_key tmp_key;
> struct avtab_node *cur;
> + int cmp;
>
> if (!node)
> return NULL;
> -
> - specified &= ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
> + tmp_key = node->key;
> + tmp_key.specified = specified;
> for (cur = node->next; cur; cur = cur->next) {
> - if (node->key.source_type == cur->key.source_type &&
> - node->key.target_type == cur->key.target_type &&
> - node->key.target_class == cur->key.target_class &&
> - (specified & cur->key.specified))
> + cmp = avtab_node_cmp(&tmp_key, &cur->key);
> + if (cmp == 0)
> return cur;
> -
> - if (node->key.source_type < cur->key.source_type)
> - break;
> - if (node->key.source_type == cur->key.source_type &&
> - node->key.target_type < cur->key.target_type)
> - break;
> - if (node->key.source_type == cur->key.source_type &&
> - node->key.target_type == cur->key.target_type &&
> - node->key.target_class < cur->key.target_class)
> + if (cmp < 0)
> break;
> }
> return NULL;
> --
> 2.41.0
>
