On Wed, 21 Feb 2024 at 22:08, James Carter <jwcart2@xxxxxxxxx> wrote:
>
> In checkpolicy, a sensitivity that has one or more aliases will
> temporarily share the mls_level_t structure with its aliases until
> a level statement is processed for the sensitivity (or one of the
> aliases) and the aliases are updated to have their own mls_level_t
> structure. If the policydb is destroyed while they are sharing the
> mls_level_t structure, then a double free of the shared mls_level_t
> will occur. This does not currently occur only because checkpolicy
> does very little clean-up before exiting.
>
> The "defined" field of the level_datum_t is set after a level
> statement is processed for a sensitivity and its aliases. This means
> that we know an alias has its own mls_level_t if the "defined" field
> is set. The double free can be avoided by not destroying the
> mls_leve_t structure for an alias unless the "defined" field is set.
>
> Since the "defined" field is only set to false while the mls_level_t
> structure is being shared, it would be clearer to rename the field
> as "notdefined". It would only be set during the time the sensitivity
> and its aliases are sharing the mls_level_t structure. Outside of
> checkpolicy, the "notdefined" field will always be set to 0.
>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> v2: Change the field name from "defined" to "notdefined" and change
> the logic to match.
Thanks, in my opinion this is a much nicer approach.
Maybe check in libsepol/src/policydb_validate.c:validate_level_datum()
that notdefined is FALSE?
> checkpolicy/checkpolicy.c | 7 +++----
> checkpolicy/policy_define.c | 10 ++++++----
> libsepol/cil/src/cil_binary.c | 3 ---
> libsepol/include/sepol/policydb/policydb.h | 2 +-
> libsepol/src/policydb.c | 6 ++++--
> 5 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
> index fcec6e51..d7cafaa4 100644
> --- a/checkpolicy/checkpolicy.c
> +++ b/checkpolicy/checkpolicy.c
> @@ -370,10 +370,9 @@ static int check_level(hashtab_key_t key, hashtab_datum_t datum, void *arg __att
> {
> level_datum_t *levdatum = (level_datum_t *) datum;
>
> - if (!levdatum->isalias && !levdatum->defined) {
> - fprintf(stderr,
> - "Error: sensitivity %s was not used in a level definition!\n",
> - key);
> + if (!levdatum->isalias && levdatum->notdefined) {
> + fprintf(stderr, "Error: sensitivity %s was not used in a level definition!\n",
> + key);
> return -1;
> }
> return 0;
> diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
> index 260e609d..ac215086 100644
> --- a/checkpolicy/policy_define.c
> +++ b/checkpolicy/policy_define.c
> @@ -743,6 +743,7 @@ int define_sens(void)
> level_datum_init(datum);
> datum->isalias = FALSE;
> datum->level = level;
> + datum->notdefined = TRUE;
>
> ret = declare_symbol(SYM_LEVELS, id, datum, &value, &value);
> switch (ret) {
> @@ -780,6 +781,7 @@ int define_sens(void)
> level_datum_init(aliasdatum);
> aliasdatum->isalias = TRUE;
> aliasdatum->level = level;
> + aliasdatum->notdefined = TRUE;
>
> ret = declare_symbol(SYM_LEVELS, id, aliasdatum, NULL, &value);
> switch (ret) {
> @@ -1006,9 +1008,10 @@ static int clone_level(hashtab_key_t key __attribute__ ((unused)), hashtab_datum
> mls_level_t *level = (mls_level_t *) arg, *newlevel;
>
> if (levdatum->level == level) {
> - levdatum->defined = 1;
> - if (!levdatum->isalias)
> + if (!levdatum->isalias) {
> + levdatum->notdefined = FALSE;
> return 0;
> + }
> newlevel = (mls_level_t *) malloc(sizeof(mls_level_t));
> if (!newlevel)
> return -1;
> @@ -1017,6 +1020,7 @@ static int clone_level(hashtab_key_t key __attribute__ ((unused)), hashtab_datum
> return -1;
> }
> levdatum->level = newlevel;
> + levdatum->notdefined = FALSE;
> }
> return 0;
> }
> @@ -1057,8 +1061,6 @@ int define_level(void)
> }
> free(id);
>
> - levdatum->defined = 1;
> -
> while ((id = queue_remove(id_queue))) {
> cat_datum_t *cdatum;
> int range_start, range_end, i;
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index a8e3616a..95bd18ba 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -907,7 +907,6 @@ static int cil_sensalias_to_policydb(policydb_t *pdb, struct cil_alias *cil_alia
> goto exit;
> }
> sepol_alias->level = mls_level;
> - sepol_alias->defined = 1;
> sepol_alias->isalias = 1;
>
> return SEPOL_OK;
> @@ -3163,8 +3162,6 @@ int cil_sepol_level_define(policydb_t *pdb, struct cil_sens *cil_sens)
> }
> }
>
> - sepol_level->defined = 1;
> -
> return SEPOL_OK;
>
> exit:
> diff --git a/libsepol/include/sepol/policydb/policydb.h b/libsepol/include/sepol/policydb/policydb.h
> index 6682069e..66d93999 100644
> --- a/libsepol/include/sepol/policydb/policydb.h
> +++ b/libsepol/include/sepol/policydb/policydb.h
> @@ -217,7 +217,7 @@ typedef struct user_datum {
> typedef struct level_datum {
> mls_level_t *level; /* sensitivity and associated categories */
> unsigned char isalias; /* is this sensitivity an alias for another? */
> - unsigned char defined;
> + unsigned char notdefined;
Maybe add a small comment that it's only used as an optimization in
checkpolicy and is 0 for fully parsed or generated policies?
> } level_datum_t;
>
> /* Category attributes */
> diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
> index f10a8a95..0c23a7a2 100644
> --- a/libsepol/src/policydb.c
> +++ b/libsepol/src/policydb.c
> @@ -1390,8 +1390,10 @@ static int sens_destroy(hashtab_key_t key, hashtab_datum_t datum, void *p
> if (key)
> free(key);
> levdatum = (level_datum_t *) datum;
> - mls_level_destroy(levdatum->level);
> - free(levdatum->level);
> + if (!levdatum->isalias || !levdatum->notdefined) {
> + mls_level_destroy(levdatum->level);
> + free(levdatum->level);
> + }
> level_datum_destroy(levdatum);
> free(levdatum);
> return 0;
> --
> 2.43.0
>
