[PATCH 1/5] libsepol/cil: declare file local function pointer static

[PATCH 1/5] libsepol/cil: declare file local function pointer static

[Christian Göttsche]

    ../cil/src/cil_log.c:45:8: warning: no previous extern declaration for non-static variable 'cil_log_handler' [-Wmissing-variable-declarations]
    void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
           ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/cil/src/cil_log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_log.c b/libsepol/cil/src/cil_log.c
index e45c58fc..f4c6e415 100644
--- a/libsepol/cil/src/cil_log.c
+++ b/libsepol/cil/src/cil_log.c
@@ -42,7 +42,7 @@ static void cil_default_log_handler(__attribute__((unused)) int lvl, const char
 	fprintf(stderr, "%s", msg);
 }
 
-void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
+static void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
 
 void cil_set_log_handler(void (*handler)(int lvl, const char *msg))
 {
-- 
2.35.1

[PATCH 2/5] libsepol: check correct pointer for oom

[Christian Göttsche]

Check the actual pointer which memory was assigned to, not its parent
array pointer.

    services.c:810:14: warning: Assigned value is garbage or undefined [core.uninitialized.Assign]
                                            **r_buf = **new_buf;
                                                    ^ ~~~~~~~~~

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/services.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/src/services.c b/libsepol/src/services.c
index 29723729..b8fb2704 100644
--- a/libsepol/src/services.c
+++ b/libsepol/src/services.c
@@ -803,7 +803,7 @@ mls_ops:
 				if (len < 0 || len >= reason_buf_len - reason_buf_used) {
 					new_buf_len = reason_buf_len + REASON_BUF_SIZE;
 					*new_buf = realloc(*r_buf, new_buf_len);
-					if (!new_buf) {
+					if (!*new_buf) {
 						ERR(NULL, "failed to realloc reason buffer");
 						goto out1;
 					}
-- 
2.35.1

[PATCH 3/5] libsepol: drop unnecessary const discarding casts

[Christian Göttsche]

`hashtab_search()` takes a const_hashtab_key_t, alias `const char*` as
second key parameter type.  Do not unnecessarily cast variables of type
`const char*` to hashtab_key_t, alias `char*`.

    policydb.c: In function ‘policydb_string_to_security_class’:
    policydb.c:4164:39: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
     4164 |                                       (hashtab_key_t) class_name);
          |                                       ^
    policydb.c: In function ‘policydb_string_to_av_perm’:
    policydb.c:4184:25: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
     4184 |                         (hashtab_key_t)perm_name);
          |                         ^
    policydb.c:4193:25: warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
     4193 |                         (hashtab_key_t)perm_name);
          |                         ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/policydb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 5c7e35e8..0c71f09f 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -4160,7 +4160,7 @@ static sepol_security_class_t policydb_string_to_security_class(
 	class_datum_t *tclass_datum;
 
 	tclass_datum = hashtab_search(policydb->p_classes.table,
-				      (hashtab_key_t) class_name);
+				      class_name);
 	if (!tclass_datum)
 		return 0;
 	return tclass_datum->s.value;
@@ -4180,7 +4180,7 @@ static sepol_access_vector_t policydb_string_to_av_perm(
 
 	perm_datum = (perm_datum_t *)
 			hashtab_search(tclass_datum->permissions.table,
-			(hashtab_key_t)perm_name);
+			perm_name);
 	if (perm_datum != NULL)
 		return UINT32_C(1) << (perm_datum->s.value - 1);
 
@@ -4189,7 +4189,7 @@ static sepol_access_vector_t policydb_string_to_av_perm(
 
 	perm_datum = (perm_datum_t *)
 			hashtab_search(tclass_datum->comdatum->permissions.table,
-			(hashtab_key_t)perm_name);
+			perm_name);
 
 	if (perm_datum != NULL)
 		return UINT32_C(1) << (perm_datum->s.value - 1);
-- 
2.35.1

[PATCH 4/5] libselinux: limit has buffer size

[Christian Göttsche]

The `struct selabel_digest` member `hashbuf_size` is used to compute
hashes via `Sha1Update()`, which takes uint32_t as length parameter
type.  Use that same type for `hashbuf_size` to avoid potential value
truncations, as the overflow check in `digest_add_specfile()` on
`hashbuf_size` is based on it.

    label_support.c: In function ‘digest_gen_hash’:
    label_support.c:125:53: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
      125 |         Sha1Update(&context, digest->hashbuf, digest->hashbuf_size);
          |                                               ~~~~~~^~~~~~~~~~~~~~

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/label_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h
index 782c6aa8..82a762f7 100644
--- a/libselinux/src/label_internal.h
+++ b/libselinux/src/label_internal.h
@@ -57,7 +57,7 @@ int selabel_service_init(struct selabel_handle *rec,
 struct selabel_digest {
 	unsigned char *digest;	/* SHA1 digest of specfiles */
 	unsigned char *hashbuf;	/* buffer to hold specfiles */
-	size_t hashbuf_size;	/* buffer size */
+	uint32_t hashbuf_size;	/* buffer size */
 	size_t specfile_cnt;	/* how many specfiles processed */
 	char **specfile_list;	/* and their names */
 };
-- 
2.35.1

[PATCH 5/5] libsemanage: avoid double fclose

[Christian Göttsche]

The cleanup goto block in `semanage_direct_set_enabled()` closes the
file stream pointer fp if not NULL.  Set the stream to NULL after a
manual fclose(3), even on failure.

    direct_api.c: In function ‘semanage_direct_set_enabled’:
    direct_api.c:2130:25: error: pointer ‘fp’ may be used after ‘fclose’ [-Werror=use-after-free]
     2130 |         if (fp != NULL) fclose(fp);
          |                         ^~~~~~~~~~
    direct_api.c:2092:29: note: call to ‘fclose’ here
     2092 |                         if (fclose(fp) != 0) {
          |                             ^~~~~~~~~~

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsemanage/src/direct_api.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libsemanage/src/direct_api.c b/libsemanage/src/direct_api.c
index d5716ce5..7206483a 100644
--- a/libsemanage/src/direct_api.c
+++ b/libsemanage/src/direct_api.c
@@ -2089,7 +2089,9 @@ static int semanage_direct_set_enabled(semanage_handle_t *sh,
 				goto cleanup;
 			}
 
-			if (fclose(fp) != 0) {
+			ret = fclose(fp);
+			fp = NULL;
+			if (ret != 0) {
 				ERR(sh,
 				    "Unable to close disabled file for module %s",
 				    modkey->name);
@@ -2097,8 +2099,6 @@ static int semanage_direct_set_enabled(semanage_handle_t *sh,
 				goto cleanup;
 			}
 
-			fp = NULL;
-
 			break;
 		case 1: /* enable the module */
 			if (unlink(fn) < 0) {
-- 
2.35.1

Re: [PATCH 4/5] libselinux: limit has buffer size

[Christian Göttsche]

On Fri, 8 Apr 2022 at 15:10, Christian Göttsche <cgzones@googlemail.com> wrote:
>
> The `struct selabel_digest` member `hashbuf_size` is used to compute
> hashes via `Sha1Update()`, which takes uint32_t as length parameter
> type.  Use that same type for `hashbuf_size` to avoid potential value
> truncations, as the overflow check in `digest_add_specfile()` on
> `hashbuf_size` is based on it.
>
>     label_support.c: In function ‘digest_gen_hash’:
>     label_support.c:125:53: warning: conversion from ‘size_t’ {aka ‘long unsigned int’} to ‘uint32_t’ {aka ‘unsigned int’} may change value [-Wconversion]
>       125 |         Sha1Update(&context, digest->hashbuf, digest->hashbuf_size);
>           |                                               ~~~~~~^~~~~~~~~~~~~~

An alternative would be to split the `Sha1Update()` call[1] into
multiple, each for a maximum of UINT32_MAX bytes.

[1]: https://github.com/SELinuxProject/selinux/blob/73562de8fc70b21aeb6be86dfdfddb7502d236ea/libselinux/src/label_support.c#L125


> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libselinux/src/label_internal.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libselinux/src/label_internal.h b/libselinux/src/label_internal.h
> index 782c6aa8..82a762f7 100644
> --- a/libselinux/src/label_internal.h
> +++ b/libselinux/src/label_internal.h
> @@ -57,7 +57,7 @@ int selabel_service_init(struct selabel_handle *rec,
>  struct selabel_digest {
>         unsigned char *digest;  /* SHA1 digest of specfiles */
>         unsigned char *hashbuf; /* buffer to hold specfiles */
> -       size_t hashbuf_size;    /* buffer size */
> +       uint32_t hashbuf_size;  /* buffer size */
>         size_t specfile_cnt;    /* how many specfiles processed */
>         char **specfile_list;   /* and their names */
>  };
> --
> 2.35.1
>

Re: [PATCH 1/5] libsepol/cil: declare file local function pointer static

[James Carter]

On Fri, Apr 8, 2022 at 9:37 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
>     ../cil/src/cil_log.c:45:8: warning: no previous extern declaration for non-static variable 'cil_log_handler' [-Wmissing-variable-declarations]
>     void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
>            ^
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

For these 5 patches:
Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_log.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_log.c b/libsepol/cil/src/cil_log.c
> index e45c58fc..f4c6e415 100644
> --- a/libsepol/cil/src/cil_log.c
> +++ b/libsepol/cil/src/cil_log.c
> @@ -42,7 +42,7 @@ static void cil_default_log_handler(__attribute__((unused)) int lvl, const char
>         fprintf(stderr, "%s", msg);
>  }
>
> -void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
> +static void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
>
>  void cil_set_log_handler(void (*handler)(int lvl, const char *msg))
>  {
> --
> 2.35.1
>

Re: [PATCH 1/5] libsepol/cil: declare file local function pointer static

[James Carter]

On Tue, Apr 12, 2022 at 9:03 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Fri, Apr 8, 2022 at 9:37 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> >     ../cil/src/cil_log.c:45:8: warning: no previous extern declaration for non-static variable 'cil_log_handler' [-Wmissing-variable-declarations]
> >     void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
> >            ^
> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> For these 5 patches:
> Acked-by: James Carter <jwcart2@gmail.com>
>

These 5 patches have been merged.
Thanks,
Jim

> > ---
> >  libsepol/cil/src/cil_log.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libsepol/cil/src/cil_log.c b/libsepol/cil/src/cil_log.c
> > index e45c58fc..f4c6e415 100644
> > --- a/libsepol/cil/src/cil_log.c
> > +++ b/libsepol/cil/src/cil_log.c
> > @@ -42,7 +42,7 @@ static void cil_default_log_handler(__attribute__((unused)) int lvl, const char
> >         fprintf(stderr, "%s", msg);
> >  }
> >
> > -void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
> > +static void (*cil_log_handler)(int lvl, const char *msg) = &cil_default_log_handler;
> >
> >  void cil_set_log_handler(void (*handler)(int lvl, const char *msg))
> >  {
> > --
> > 2.35.1
> >