* [PATCH 01/13] libsepol: avoid implicit conversions
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 02/13] libsepol: free memory after policy validation Christian Göttsche
` (12 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.
util.c:95:15: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
libsepol/src/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libsepol/src/util.c b/libsepol/src/util.c
index a47cae87..902c63c5 100644
--- a/libsepol/src/util.c
+++ b/libsepol/src/util.c
@@ -92,7 +92,7 @@ char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
cladatum = policydbp->class_val_to_struct[tclass - 1];
p = avbuf;
for (i = 0; i < cladatum->permissions.nprim; i++) {
- if (av & (1 << i)) {
+ if (av & (UINT32_C(1) << i)) {
v.val = i + 1;
rc = hashtab_map(cladatum->permissions.table,
perm_name, &v);
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 02/13] libsepol: free memory after policy validation
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
2021-09-14 12:48 ` [PATCH 01/13] libsepol: avoid implicit conversions Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-15 13:11 ` [PATCH v2 " Christian Göttsche
2021-09-14 12:48 ` [PATCH 03/13] checkpolicy: enclose macro argument in parentheses Christian Göttsche
` (11 subsequent siblings)
13 siblings, 1 reply; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Found while running the checkpolicy/test/dispol binary.
Direct leak of 24 byte(s) in 1 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
#1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
#2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
#3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
#4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
#5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
#6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
#7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
#8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Indirect leak of 48 byte(s) in 2 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
#1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
#2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
#3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
#4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
#5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
#6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
#7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
#8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
libsepol/src/policydb_validate.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
index 246aa6e3..f9a4b942 100644
--- a/libsepol/src/policydb_validate.c
+++ b/libsepol/src/policydb_validate.c
@@ -722,12 +722,21 @@ bad:
return -1;
}
+static void validate_destroy(validate_t flavors[])
+{
+ unsigned int i;
+
+ for (i = 0; i < SYM_NUM; i++) {
+ ebitmap_destroy(&flavors[i].gaps);
+ }
+}
+
/*
* Validate policydb
*/
int validate_policydb(sepol_handle_t *handle, policydb_t *p)
{
- validate_t flavors[SYM_NUM];
+ validate_t flavors[SYM_NUM] = {};
if (validate_array_init(p, flavors))
goto bad;
@@ -756,9 +765,12 @@ int validate_policydb(sepol_handle_t *handle, policydb_t *p)
if (validate_datum_arrays(handle, p, flavors))
goto bad;
+ validate_destroy(flavors);
+
return 0;
bad:
ERR(handle, "Invalid policydb");
+ validate_destroy(flavors);
return -1;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 02/13] libsepol: free memory after policy validation
2021-09-14 12:48 ` [PATCH 02/13] libsepol: free memory after policy validation Christian Göttsche
@ 2021-09-15 13:11 ` Christian Göttsche
2021-09-15 13:19 ` [PATCH v3 " Christian Göttsche
0 siblings, 1 reply; 22+ messages in thread
From: Christian Göttsche @ 2021-09-15 13:11 UTC (permalink / raw)
To: selinux
Found while running the checkpolicy/test/dispol binary.
Direct leak of 24 byte(s) in 1 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
#1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
#2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
#3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
#4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
#5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
#6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
#7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
#8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Indirect leak of 48 byte(s) in 2 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
#1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
#2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
#3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
#4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
#5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
#6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
#7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
#8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
- name function validate_array_destroy(), similar to the allocating
validate_array_init()
libsepol/src/policydb_validate.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
index 246aa6e3..3ab072a8 100644
--- a/libsepol/src/policydb_validate.c
+++ b/libsepol/src/policydb_validate.c
@@ -722,12 +722,21 @@ bad:
return -1;
}
+static void validate_array_destroy(validate_t flavors[])
+{
+ unsigned int i;
+
+ for (i = 0; i < SYM_NUM; i++) {
+ ebitmap_destroy(&flavors[i].gaps);
+ }
+}
+
/*
* Validate policydb
*/
int validate_policydb(sepol_handle_t *handle, policydb_t *p)
{
- validate_t flavors[SYM_NUM];
+ validate_t flavors[SYM_NUM] = {};
if (validate_array_init(p, flavors))
goto bad;
@@ -756,9 +765,12 @@ int validate_policydb(sepol_handle_t *handle, policydb_t *p)
if (validate_datum_arrays(handle, p, flavors))
goto bad;
+ validate_array_destroy(flavors);
+
return 0;
bad:
ERR(handle, "Invalid policydb");
+ validate_array_destroy(flavors);
return -1;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v3 02/13] libsepol: free memory after policy validation
2021-09-15 13:11 ` [PATCH v2 " Christian Göttsche
@ 2021-09-15 13:19 ` Christian Göttsche
0 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-15 13:19 UTC (permalink / raw)
To: selinux
Found while running the checkpolicy/test/dispol binary.
Direct leak of 24 byte(s) in 1 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
#1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
#2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
#3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
#4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
#5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
#6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
#7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
#8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Indirect leak of 48 byte(s) in 2 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
#1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
#2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
#3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
#4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
#5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
#6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
#7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
#8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
- fix indentation
v2:
- name function validate_array_destroy(), similar to the allocating
validate_array_init()
libsepol/src/policydb_validate.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
index 246aa6e3..5804d247 100644
--- a/libsepol/src/policydb_validate.c
+++ b/libsepol/src/policydb_validate.c
@@ -722,12 +722,21 @@ bad:
return -1;
}
+static void validate_array_destroy(validate_t flavors[])
+{
+ unsigned int i;
+
+ for (i = 0; i < SYM_NUM; i++) {
+ ebitmap_destroy(&flavors[i].gaps);
+ }
+}
+
/*
* Validate policydb
*/
int validate_policydb(sepol_handle_t *handle, policydb_t *p)
{
- validate_t flavors[SYM_NUM];
+ validate_t flavors[SYM_NUM] = {};
if (validate_array_init(p, flavors))
goto bad;
@@ -756,9 +765,12 @@ int validate_policydb(sepol_handle_t *handle, policydb_t *p)
if (validate_datum_arrays(handle, p, flavors))
goto bad;
+ validate_array_destroy(flavors);
+
return 0;
bad:
ERR(handle, "Invalid policydb");
+ validate_array_destroy(flavors);
return -1;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 03/13] checkpolicy: enclose macro argument in parentheses
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
2021-09-14 12:48 ` [PATCH 01/13] libsepol: avoid implicit conversions Christian Göttsche
2021-09-14 12:48 ` [PATCH 02/13] libsepol: free memory after policy validation Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 04/13] checkpolicy: misc checkmodule tweaks Christian Göttsche
` (10 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Found by clang-tidy
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/policy_define.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 75a67d5c..7e7801d3 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -2143,13 +2143,13 @@ out:
}
/* index of the u32 containing the permission */
-#define XPERM_IDX(x) (x >> 5)
+#define XPERM_IDX(x) ((x) >> 5)
/* set bits 0 through x-1 within the u32 */
-#define XPERM_SETBITS(x) ((1U << (x & 0x1f)) - 1)
+#define XPERM_SETBITS(x) ((1U << ((x) & 0x1f)) - 1)
/* low value for this u32 */
-#define XPERM_LOW(x) (x << 5)
+#define XPERM_LOW(x) ((x) << 5)
/* high value for this u32 */
-#define XPERM_HIGH(x) (((x + 1) << 5) - 1)
+#define XPERM_HIGH(x) ((((x) + 1) << 5) - 1)
void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
av_extended_perms_t *xperms)
{
@@ -2189,9 +2189,9 @@ int avrule_xperms_used(const av_extended_perms_t *xperms)
* dir, size, driver, and function. Only the driver and function fields
* are considered here
*/
-#define IOC_DRIV(x) (x >> 8)
-#define IOC_FUNC(x) (x & 0xff)
-#define IOC_CMD(driver, func) ((driver << 8) + func)
+#define IOC_DRIV(x) ((x) >> 8)
+#define IOC_FUNC(x) ((x) & 0xff)
+#define IOC_CMD(driver, func) (((driver) << 8) + (func))
int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
av_extended_perms_t *complete_driver,
av_extended_perms_t **extended_perms)
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 04/13] checkpolicy: misc checkmodule tweaks
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (2 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 03/13] checkpolicy: enclose macro argument in parentheses Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 05/13] checkpolicy: misc checkpolicy tweaks Christian Göttsche
` (9 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Add missing argument in usage message.
Drop redundant includes `optarg` and `optind`, which are declared in
<getopt.h>.
Use consistent quit style by using `exit(1)`.
Mark read-only options struct const.
Check closing file streams after writing, which can signal a failed
write or sync to disk and should be checked.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/checkmodule.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/checkpolicy/checkmodule.c b/checkpolicy/checkmodule.c
index 316b2898..3432608b 100644
--- a/checkpolicy/checkmodule.c
+++ b/checkpolicy/checkmodule.c
@@ -34,9 +34,6 @@
#include "checkpolicy.h"
#include "parse_util.h"
-extern char *optarg;
-extern int optind;
-
static sidtab_t sidtab;
extern int mlspol;
@@ -126,7 +123,7 @@ static int write_binary_policy(policydb_t * p, FILE *outfp)
static __attribute__((__noreturn__)) void usage(const char *progname)
{
- printf("usage: %s [-h] [-V] [-b] [-C] [-E] [-U handle_unknown] [-m] [-M] [-o FILE] [INPUT]\n", progname);
+ printf("usage: %s [-h] [-V] [-b] [-C] [-E] [-U handle_unknown] [-m] [-M] [-o FILE] [-c VERSION] [INPUT]\n", progname);
printf("Build base and policy modules.\n");
printf("Options:\n");
printf(" INPUT build module from INPUT (else read from \"%s\")\n",
@@ -155,7 +152,7 @@ int main(int argc, char **argv)
int ch;
int show_version = 0;
policydb_t modpolicydb;
- struct option long_options[] = {
+ const struct option long_options[] = {
{"help", no_argument, NULL, 'h'},
{"output", required_argument, NULL, 'o'},
{"binary", no_argument, NULL, 'b'},
@@ -271,7 +268,7 @@ int main(int argc, char **argv)
} else {
if (policydb_init(&modpolicydb)) {
fprintf(stderr, "%s: out of memory!\n", argv[0]);
- return -1;
+ exit(1);
}
modpolicydb.policy_type = policy_type;
@@ -283,7 +280,7 @@ int main(int argc, char **argv)
}
if (hierarchy_check_constraints(NULL, &modpolicydb)) {
- return -1;
+ exit(1);
}
}
@@ -336,7 +333,7 @@ int main(int argc, char **argv)
FILE *outfp = fopen(outfile, "w");
if (!outfp) {
- perror(outfile);
+ fprintf(stderr, "%s: error opening %s: %s\n", argv[0], outfile, strerror(errno));
exit(1);
}
@@ -352,7 +349,10 @@ int main(int argc, char **argv)
}
}
- fclose(outfp);
+ if (fclose(outfp)) {
+ fprintf(stderr, "%s: error closing %s: %s\n", argv[0], outfile, strerror(errno));
+ exit(1);
+ }
} else if (cil) {
fprintf(stderr, "%s: No file to write CIL was specified\n", argv[0]);
exit(1);
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 05/13] checkpolicy: misc checkpolicy tweaks
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (3 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 04/13] checkpolicy: misc checkmodule tweaks Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 06/13] checkpolicy: mark read-only parameters in module compiler const Christian Göttsche
` (8 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Add missing argument in usage message.
Drop redundant includes `optarg` and `optind`, which are declared in
<getopt.h>.
Mark file local functions static.
Drop unused function declaration.
Check closing file streams after writing, which can signal a failed
write or sync to disk and should be checked.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/checkpolicy.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
index e6cfd337..9459486b 100644
--- a/checkpolicy/checkpolicy.c
+++ b/checkpolicy/checkpolicy.c
@@ -92,9 +92,6 @@
#include "checkpolicy.h"
#include "parse_util.h"
-extern char *optarg;
-extern int optind;
-
static policydb_t policydb;
static sidtab_t sidtab;
@@ -112,7 +109,7 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
{
printf
("usage: %s [-b[F]] [-C] [-d] [-U handle_unknown (allow,deny,reject)] [-M] "
- "[-c policyvers (%d-%d)] [-o output_file|-] [-S] "
+ "[-c policyvers (%d-%d)] [-o output_file|-] [-S] [-O]"
"[-t target_platform (selinux,xen)] [-E] [-V] [input_file]\n",
progname, POLICYDB_VERSION_MIN, POLICYDB_VERSION_MAX);
exit(1);
@@ -297,9 +294,7 @@ static int identify_equiv_types(void)
}
#endif
-extern char *av_to_string(uint32_t tclass, sepol_access_vector_t av);
-
-int display_bools(void)
+static int display_bools(void)
{
uint32_t i;
@@ -310,10 +305,10 @@ int display_bools(void)
return 0;
}
-void display_expr(cond_expr_t * exp)
+static void display_expr(const cond_expr_t * exp)
{
- cond_expr_t *cur;
+ const cond_expr_t *cur;
for (cur = exp; cur != NULL; cur = cur->next) {
switch (cur->expr_type) {
case COND_BOOL:
@@ -345,9 +340,9 @@ void display_expr(cond_expr_t * exp)
}
}
-int display_cond_expressions(void)
+static int display_cond_expressions(void)
{
- cond_node_t *cur;
+ const cond_node_t *cur;
for (cur = policydbp->cond_list; cur != NULL; cur = cur->next) {
printf("expression: ");
@@ -357,7 +352,7 @@ int display_cond_expressions(void)
return 0;
}
-int change_bool(char *name, int state)
+static int change_bool(const char *name, int state)
{
cond_bool_datum_t *bool;
@@ -412,7 +407,7 @@ int main(int argc, char **argv)
unsigned int reason;
int flags;
struct policy_file pf;
- struct option long_options[] = {
+ const struct option long_options[] = {
{"output", required_argument, NULL, 'o'},
{"target", required_argument, NULL, 't'},
{"binary", no_argument, NULL, 'b'},
@@ -706,7 +701,10 @@ int main(int argc, char **argv)
}
if (outfp != stdout) {
- fclose(outfp);
+ if(fclose(outfp)) {
+ fprintf(stderr, "%s: error closing %s: %s\n", argv[0], outfile, strerror(errno));
+ exit(1);
+ }
}
} else if (cil) {
fprintf(stderr, "%s: No file to write CIL was specified\n", argv[0]);
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 06/13] checkpolicy: mark read-only parameters in module compiler const
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (4 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 05/13] checkpolicy: misc checkpolicy tweaks Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 07/13] checkpolicy: mark file local functions in policy_define static Christian Göttsche
` (7 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Make it more obvious which parameters are read-only and not being
modified and allow callers to pass const pointers.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/module_compiler.c | 26 +++++++++++++-------------
checkpolicy/module_compiler.h | 4 ++--
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/checkpolicy/module_compiler.c b/checkpolicy/module_compiler.c
index ada7cb2a..e8f15f4e 100644
--- a/checkpolicy/module_compiler.c
+++ b/checkpolicy/module_compiler.c
@@ -1104,14 +1104,14 @@ int require_cat(int pass)
return 0;
}
-static int is_scope_in_stack(scope_datum_t * scope, scope_stack_t * stack)
+static int is_scope_in_stack(const scope_datum_t * scope, const scope_stack_t * stack)
{
uint32_t i;
if (stack == NULL) {
return 0; /* no matching scope found */
}
if (stack->type == 1) {
- avrule_decl_t *decl = stack->decl;
+ const avrule_decl_t *decl = stack->decl;
for (i = 0; i < scope->decl_ids_len; i++) {
if (scope->decl_ids[i] == decl->decl_id) {
return 1;
@@ -1126,9 +1126,9 @@ static int is_scope_in_stack(scope_datum_t * scope, scope_stack_t * stack)
return is_scope_in_stack(scope, stack->parent);
}
-int is_id_in_scope(uint32_t symbol_type, hashtab_key_t id)
+int is_id_in_scope(uint32_t symbol_type, const_hashtab_key_t id)
{
- scope_datum_t *scope =
+ const scope_datum_t *scope =
(scope_datum_t *) hashtab_search(policydbp->scope[symbol_type].
table, id);
if (scope == NULL) {
@@ -1138,7 +1138,7 @@ int is_id_in_scope(uint32_t symbol_type, hashtab_key_t id)
}
static int is_perm_in_scope_index(uint32_t perm_value, uint32_t class_value,
- scope_index_t * scope)
+ const scope_index_t * scope)
{
if (class_value > scope->class_perms_len) {
return 1;
@@ -1151,7 +1151,7 @@ static int is_perm_in_scope_index(uint32_t perm_value, uint32_t class_value,
}
static int is_perm_in_stack(uint32_t perm_value, uint32_t class_value,
- scope_stack_t * stack)
+ const scope_stack_t * stack)
{
if (stack == NULL) {
return 0; /* no matching scope found */
@@ -1173,12 +1173,12 @@ static int is_perm_in_stack(uint32_t perm_value, uint32_t class_value,
return is_perm_in_stack(perm_value, class_value, stack->parent);
}
-int is_perm_in_scope(hashtab_key_t perm_id, hashtab_key_t class_id)
+int is_perm_in_scope(const_hashtab_key_t perm_id, const_hashtab_key_t class_id)
{
- class_datum_t *cladatum =
+ const class_datum_t *cladatum =
(class_datum_t *) hashtab_search(policydbp->p_classes.table,
class_id);
- perm_datum_t *perdatum;
+ const perm_datum_t *perdatum;
if (cladatum == NULL) {
return 1;
}
@@ -1361,17 +1361,17 @@ int begin_optional_else(int pass)
return 0;
}
-static int copy_requirements(avrule_decl_t * dest, scope_stack_t * stack)
+static int copy_requirements(avrule_decl_t * dest, const scope_stack_t * stack)
{
uint32_t i;
if (stack == NULL) {
return 0;
}
if (stack->type == 1) {
- scope_index_t *src_scope = &stack->decl->required;
+ const scope_index_t *src_scope = &stack->decl->required;
scope_index_t *dest_scope = &dest->required;
for (i = 0; i < SYM_NUM; i++) {
- ebitmap_t *src_bitmap = &src_scope->scope[i];
+ const ebitmap_t *src_bitmap = &src_scope->scope[i];
ebitmap_t *dest_bitmap = &dest_scope->scope[i];
if (ebitmap_union(dest_bitmap, src_bitmap)) {
yyerror("Out of memory!");
@@ -1397,7 +1397,7 @@ static int copy_requirements(avrule_decl_t * dest, scope_stack_t * stack)
src_scope->class_perms_len;
}
for (i = 0; i < src_scope->class_perms_len; i++) {
- ebitmap_t *src_bitmap = &src_scope->class_perms_map[i];
+ const ebitmap_t *src_bitmap = &src_scope->class_perms_map[i];
ebitmap_t *dest_bitmap =
&dest_scope->class_perms_map[i];
if (ebitmap_union(dest_bitmap, src_bitmap)) {
diff --git a/checkpolicy/module_compiler.h b/checkpolicy/module_compiler.h
index 72c2d9bb..29b824b4 100644
--- a/checkpolicy/module_compiler.h
+++ b/checkpolicy/module_compiler.h
@@ -65,12 +65,12 @@ int require_cat(int pass);
/* Check if an identifier is within the scope of the current
* declaration or any of its parents. Return 1 if it is, 0 if not.
* If the identifier is not known at all then return 1 (truth). */
-int is_id_in_scope(uint32_t symbol_type, hashtab_key_t id);
+int is_id_in_scope(uint32_t symbol_type, const_hashtab_key_t id);
/* Check if a particular permission is within the scope of the current
* declaration or any of its parents. Return 1 if it is, 0 if not.
* If the identifier is not known at all then return 1 (truth). */
-int is_perm_in_scope(hashtab_key_t perm_id, hashtab_key_t class_id);
+int is_perm_in_scope(const_hashtab_key_t perm_id, const_hashtab_key_t class_id);
/* Search the current avrules block for a conditional with the same
* expression as 'cond'. If the conditional does not exist then
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 07/13] checkpolicy: mark file local functions in policy_define static
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (5 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 06/13] checkpolicy: mark read-only parameters in module compiler const Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 08/13] checkpolicy: add missing function declarations Christian Göttsche
` (6 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Also remove the unused function `avrule_ioctl_freeranges()`.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/policy_define.c | 45 ++++++++++++++-----------------------
1 file changed, 17 insertions(+), 28 deletions(-)
diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 7e7801d3..11707700 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -1605,7 +1605,7 @@ static int set_types(type_set_t * set, char *id, int *add, char starallowed)
return -1;
}
-int define_compute_type_helper(int which, avrule_t ** rule)
+static int define_compute_type_helper(int which, avrule_t ** rule)
{
char *id;
type_datum_t *datum;
@@ -1832,7 +1832,7 @@ struct av_ioctl_range_list {
struct av_ioctl_range_list *next;
};
-int avrule_sort_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_sort_ioctls(struct av_ioctl_range_list **rangehead)
{
struct av_ioctl_range_list *r, *r2, *sorted, *sortedhead = NULL;
@@ -1880,7 +1880,7 @@ error:
return -1;
}
-int avrule_merge_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_merge_ioctls(struct av_ioctl_range_list **rangehead)
{
struct av_ioctl_range_list *r, *tmp;
r = *rangehead;
@@ -1900,7 +1900,7 @@ int avrule_merge_ioctls(struct av_ioctl_range_list **rangehead)
return 0;
}
-int avrule_read_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_read_ioctls(struct av_ioctl_range_list **rangehead)
{
char *id;
struct av_ioctl_range_list *rnew, *r = NULL;
@@ -1953,7 +1953,7 @@ error:
}
/* flip to included ranges */
-int avrule_omit_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_omit_ioctls(struct av_ioctl_range_list **rangehead)
{
struct av_ioctl_range_list *rnew, *r, *newhead, *r2;
@@ -2001,7 +2001,7 @@ error:
return -1;
}
-int avrule_ioctl_ranges(struct av_ioctl_range_list **rangelist)
+static int avrule_ioctl_ranges(struct av_ioctl_range_list **rangelist)
{
struct av_ioctl_range_list *rangehead;
uint8_t omit;
@@ -2029,7 +2029,7 @@ int avrule_ioctl_ranges(struct av_ioctl_range_list **rangelist)
return 0;
}
-int define_te_avtab_xperms_helper(int which, avrule_t ** rule)
+static int define_te_avtab_xperms_helper(int which, avrule_t ** rule)
{
char *id;
class_perm_node_t *perms, *tail = NULL, *cur_perms = NULL;
@@ -2150,7 +2150,7 @@ out:
#define XPERM_LOW(x) ((x) << 5)
/* high value for this u32 */
#define XPERM_HIGH(x) ((((x) + 1) << 5) - 1)
-void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
+static void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
av_extended_perms_t *xperms)
{
unsigned int i;
@@ -2172,7 +2172,7 @@ void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
}
}
-int avrule_xperms_used(const av_extended_perms_t *xperms)
+static int avrule_xperms_used(const av_extended_perms_t *xperms)
{
unsigned int i;
@@ -2192,7 +2192,7 @@ int avrule_xperms_used(const av_extended_perms_t *xperms)
#define IOC_DRIV(x) ((x) >> 8)
#define IOC_FUNC(x) ((x) & 0xff)
#define IOC_CMD(driver, func) (((driver) << 8) + (func))
-int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
+static int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
av_extended_perms_t *complete_driver,
av_extended_perms_t **extended_perms)
{
@@ -2231,7 +2231,7 @@ int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
}
-int avrule_ioctl_completedriver(struct av_ioctl_range_list *rangelist,
+static int avrule_ioctl_completedriver(struct av_ioctl_range_list *rangelist,
av_extended_perms_t **extended_perms)
{
struct av_ioctl_range_list *r;
@@ -2273,7 +2273,7 @@ int avrule_ioctl_completedriver(struct av_ioctl_range_list *rangelist,
return 0;
}
-int avrule_ioctl_func(struct av_ioctl_range_list *rangelist,
+static int avrule_ioctl_func(struct av_ioctl_range_list *rangelist,
av_extended_perms_t **extended_perms, unsigned int driver)
{
struct av_ioctl_range_list *r;
@@ -2323,18 +2323,7 @@ int avrule_ioctl_func(struct av_ioctl_range_list *rangelist,
return 0;
}
-void avrule_ioctl_freeranges(struct av_ioctl_range_list *rangelist)
-{
- struct av_ioctl_range_list *r, *tmp;
- r = rangelist;
- while (r) {
- tmp = r;
- r = r->next;
- free(tmp);
- }
-}
-
-unsigned int xperms_for_each_bit(unsigned int *bit, av_extended_perms_t *xperms)
+static unsigned int xperms_for_each_bit(unsigned int *bit, av_extended_perms_t *xperms)
{
unsigned int i;
for (i = *bit; i < sizeof(xperms->perms)*8; i++) {
@@ -2347,7 +2336,7 @@ unsigned int xperms_for_each_bit(unsigned int *bit, av_extended_perms_t *xperms)
return 0;
}
-int avrule_cpy(avrule_t *dest, const avrule_t *src)
+static int avrule_cpy(avrule_t *dest, const avrule_t *src)
{
class_perm_node_t *src_perms;
class_perm_node_t *dest_perms, *dest_tail;
@@ -2395,7 +2384,7 @@ int avrule_cpy(avrule_t *dest, const avrule_t *src)
return 0;
}
-int define_te_avtab_ioctl(const avrule_t *avrule_template)
+static int define_te_avtab_ioctl(const avrule_t *avrule_template)
{
avrule_t *avrule;
struct av_ioctl_range_list *rangelist;
@@ -2490,7 +2479,7 @@ int define_te_avtab_extended_perms(int which)
return 0;
}
-int define_te_avtab_helper(int which, avrule_t ** rule)
+static int define_te_avtab_helper(int which, avrule_t ** rule)
{
char *id;
class_datum_t *cladatum;
@@ -5470,7 +5459,7 @@ int define_fs_use(int behavior)
return 0;
}
-int define_genfs_context_helper(char *fstype, int has_type)
+static int define_genfs_context_helper(char *fstype, int has_type)
{
struct genfs *genfs_p, *genfs, *newgenfs;
ocontext_t *newc, *c, *head, *p;
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 08/13] checkpolicy: add missing function declarations
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (6 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 07/13] checkpolicy: mark file local functions in policy_define static Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 09/13] checkpolicy: resolve dismod memory leaks Christian Göttsche
` (5 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Ideally they should be declared in the corresponding header file, but
the overall include style in the checkpolicy code is quite messy.
Declare them for now in the source file before defining them to silence
related compiler warnings:
policy_define.c:84:6: error: no previous prototype for function 'init_parser' [-Werror,-Wmissing-prototypes]
void init_parser(int pass_number)
^
policy_define.c:93:6: error: no previous prototype for function 'yyerror2' [-Werror,-Wmissing-prototypes]
void yyerror2(const char *fmt, ...)
^
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/policy_define.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 11707700..c71e0571 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -60,6 +60,10 @@
#include "module_compiler.h"
#include "policy_define.h"
+extern void init_parser(int pass_number);
+__attribute__ ((format(printf, 1, 2)))
+extern void yyerror2(const char *fmt, ...);
+
policydb_t *policydbp;
queue_t id_queue = 0;
unsigned int pass;
@@ -89,7 +93,6 @@ void init_parser(int pass_number)
pass = pass_number;
}
-__attribute__ ((format(printf, 1, 2)))
void yyerror2(const char *fmt, ...)
{
va_list ap;
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 09/13] checkpolicy: resolve dismod memory leaks
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (7 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 08/13] checkpolicy: add missing function declarations Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 19:45 ` James Carter
2021-09-15 13:11 ` [PATCH v2 " Christian Göttsche
2021-09-14 12:48 ` [PATCH 10/13] checkpolicy: avoid implicit conversion Christian Göttsche
` (4 subsequent siblings)
13 siblings, 2 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Example leak:
Indirect leak of 4 byte(s) in 1 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dismod+0x49bacd)
#1 0x58ae54 in add_i_to_a ./libsepol/src/util.c:55:21
#2 0x53ea8e in symtab_insert ./libsepol/src/policydb.c:1729:6
#3 0x536252 in roles_init ./libsepol/src/policydb.c:772:7
#4 0x536252 in policydb_init ./libsepol/src/policydb.c:892:7
#5 0x562ff1 in sepol_policydb_create ./libsepol/src/policydb_public.c:69:6
#6 0x521a7c in module_package_init ./libsepol/src/module.c:96:6
#7 0x521a7c in sepol_module_package_create ./libsepol/src/module.c:126:7
#8 0x4cfb80 in read_policy ./checkpolicy/test/dismod.c:750:7
#9 0x4cda10 in main ./checkpolicy/test/dismod.c:878:6
#10 0x7f8538d01e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/test/dismod.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
index 90c29318..9550b999 100644
--- a/checkpolicy/test/dismod.c
+++ b/checkpolicy/test/dismod.c
@@ -751,12 +751,15 @@ static int read_policy(char *filename, policydb_t * policy)
fprintf(stderr, "%s: Out of memory!\n", __FUNCTION__);
exit(1);
}
+ policydb_destroy((policydb_t *) package->policy);
+ free(package->policy);
package->policy = (sepol_policydb_t *) policy;
package->file_contexts = NULL;
retval =
sepol_module_package_read(package,
(sepol_policy_file_t *) & f, 1);
- free(package->file_contexts);
+ package->policy = NULL;
+ sepol_module_package_free(package);
} else {
if (policydb_init(policy)) {
fprintf(stderr, "%s: Out of memory!\n", __FUNCTION__);
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 09/13] checkpolicy: resolve dismod memory leaks
2021-09-14 12:48 ` [PATCH 09/13] checkpolicy: resolve dismod memory leaks Christian Göttsche
@ 2021-09-14 19:45 ` James Carter
2021-09-15 13:11 ` [PATCH v2 " Christian Göttsche
1 sibling, 0 replies; 22+ messages in thread
From: James Carter @ 2021-09-14 19:45 UTC (permalink / raw)
To: Christian Göttsche; +Cc: SElinux list
On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Example leak:
>
> Indirect leak of 4 byte(s) in 1 object(s) allocated from:
> #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dismod+0x49bacd)
> #1 0x58ae54 in add_i_to_a ./libsepol/src/util.c:55:21
> #2 0x53ea8e in symtab_insert ./libsepol/src/policydb.c:1729:6
> #3 0x536252 in roles_init ./libsepol/src/policydb.c:772:7
> #4 0x536252 in policydb_init ./libsepol/src/policydb.c:892:7
> #5 0x562ff1 in sepol_policydb_create ./libsepol/src/policydb_public.c:69:6
> #6 0x521a7c in module_package_init ./libsepol/src/module.c:96:6
> #7 0x521a7c in sepol_module_package_create ./libsepol/src/module.c:126:7
> #8 0x4cfb80 in read_policy ./checkpolicy/test/dismod.c:750:7
> #9 0x4cda10 in main ./checkpolicy/test/dismod.c:878:6
> #10 0x7f8538d01e49 in __libc_start_main csu/../csu/libc-start.c:314:16
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> checkpolicy/test/dismod.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
> index 90c29318..9550b999 100644
> --- a/checkpolicy/test/dismod.c
> +++ b/checkpolicy/test/dismod.c
> @@ -751,12 +751,15 @@ static int read_policy(char *filename, policydb_t * policy)
> fprintf(stderr, "%s: Out of memory!\n", __FUNCTION__);
> exit(1);
> }
> + policydb_destroy((policydb_t *) package->policy);
> + free(package->policy);
Should use "sepol_policydb_free(package->policy)" here. It does both
of these lines.
Jim
> package->policy = (sepol_policydb_t *) policy;
> package->file_contexts = NULL;
> retval =
> sepol_module_package_read(package,
> (sepol_policy_file_t *) & f, 1);
> - free(package->file_contexts);
> + package->policy = NULL;
> + sepol_module_package_free(package);
> } else {
> if (policydb_init(policy)) {
> fprintf(stderr, "%s: Out of memory!\n", __FUNCTION__);
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 09/13] checkpolicy: resolve dismod memory leaks
2021-09-14 12:48 ` [PATCH 09/13] checkpolicy: resolve dismod memory leaks Christian Göttsche
2021-09-14 19:45 ` James Carter
@ 2021-09-15 13:11 ` Christian Göttsche
1 sibling, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-15 13:11 UTC (permalink / raw)
To: selinux
Example leak:
Indirect leak of 4 byte(s) in 1 object(s) allocated from:
#0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dismod+0x49bacd)
#1 0x58ae54 in add_i_to_a ./libsepol/src/util.c:55:21
#2 0x53ea8e in symtab_insert ./libsepol/src/policydb.c:1729:6
#3 0x536252 in roles_init ./libsepol/src/policydb.c:772:7
#4 0x536252 in policydb_init ./libsepol/src/policydb.c:892:7
#5 0x562ff1 in sepol_policydb_create ./libsepol/src/policydb_public.c:69:6
#6 0x521a7c in module_package_init ./libsepol/src/module.c:96:6
#7 0x521a7c in sepol_module_package_create ./libsepol/src/module.c:126:7
#8 0x4cfb80 in read_policy ./checkpolicy/test/dismod.c:750:7
#9 0x4cda10 in main ./checkpolicy/test/dismod.c:878:6
#10 0x7f8538d01e49 in __libc_start_main csu/../csu/libc-start.c:314:16
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
- use sepol_policydb_free()
checkpolicy/test/dismod.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
index 90c29318..1ff161da 100644
--- a/checkpolicy/test/dismod.c
+++ b/checkpolicy/test/dismod.c
@@ -751,12 +751,14 @@ static int read_policy(char *filename, policydb_t * policy)
fprintf(stderr, "%s: Out of memory!\n", __FUNCTION__);
exit(1);
}
+ sepol_policydb_free(package->policy);
package->policy = (sepol_policydb_t *) policy;
package->file_contexts = NULL;
retval =
sepol_module_package_read(package,
(sepol_policy_file_t *) & f, 1);
- free(package->file_contexts);
+ package->policy = NULL;
+ sepol_module_package_free(package);
} else {
if (policydb_init(policy)) {
fprintf(stderr, "%s: Out of memory!\n", __FUNCTION__);
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 10/13] checkpolicy: avoid implicit conversion
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (8 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 09/13] checkpolicy: resolve dismod memory leaks Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 11/13] checkpolicy: error out on parsing too big integers Christian Göttsche
` (3 subsequent siblings)
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.
dismod.c:92:42: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/test/dismod.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
index 9550b999..792deb3a 100644
--- a/checkpolicy/test/dismod.c
+++ b/checkpolicy/test/dismod.c
@@ -89,7 +89,7 @@ static void render_access_bitmap(ebitmap_t * map, uint32_t class,
fprintf(fp, "{");
for (i = ebitmap_startbit(map); i < ebitmap_length(map); i++) {
if (ebitmap_get_bit(map, i)) {
- perm = sepol_av_to_string(p, class, 1 << i);
+ perm = sepol_av_to_string(p, class, UINT32_C(1) << i);
if (perm)
fprintf(fp, " %s", perm);
}
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 11/13] checkpolicy: error out on parsing too big integers
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (9 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 10/13] checkpolicy: avoid implicit conversion Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 20:43 ` James Carter
2021-09-15 13:11 ` [PATCH v2 " Christian Göttsche
2021-09-14 12:48 ` [PATCH 12/13] checkpolicy: print warning on source line overflow Christian Göttsche
` (2 subsequent siblings)
13 siblings, 2 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
Error out instead of silently converting too big integer values in
policy sources.
policy_parse.y:893:41: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/policy_parse.y | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/checkpolicy/policy_parse.y b/checkpolicy/policy_parse.y
index 6098eb50..e969d973 100644
--- a/checkpolicy/policy_parse.y
+++ b/checkpolicy/policy_parse.y
@@ -890,10 +890,22 @@ filename : FILENAME
{ yytext[strlen(yytext) - 1] = '\0'; if (insert_id(yytext + 1,0)) return -1; }
;
number : NUMBER
- { $$ = strtoul(yytext,NULL,0); }
+ { unsigned long x;
+ errno = 0;
+ x = strtoul(yytext, NULL, 0);
+ if (errno || x > UINT_MAX)
+ return -1;
+ $$ = (unsigned int) x;
+ }
;
number64 : NUMBER
- { $$ = strtoull(yytext,NULL,0); }
+ { unsigned long long x;
+ errno = 0;
+ x = strtoull(yytext, NULL, 0);
+ if (errno)
+ return -1;
+ $$ = (uint64_t) x;
+ }
;
ipv6_addr : IPV6_ADDR
{ if (insert_id(yytext,0)) return -1; }
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 11/13] checkpolicy: error out on parsing too big integers
2021-09-14 12:48 ` [PATCH 11/13] checkpolicy: error out on parsing too big integers Christian Göttsche
@ 2021-09-14 20:43 ` James Carter
2021-09-15 13:11 ` [PATCH v2 " Christian Göttsche
1 sibling, 0 replies; 22+ messages in thread
From: James Carter @ 2021-09-14 20:43 UTC (permalink / raw)
To: Christian Göttsche; +Cc: SElinux list
On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Error out instead of silently converting too big integer values in
> policy sources.
>
> policy_parse.y:893:41: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> checkpolicy/policy_parse.y | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/checkpolicy/policy_parse.y b/checkpolicy/policy_parse.y
> index 6098eb50..e969d973 100644
> --- a/checkpolicy/policy_parse.y
> +++ b/checkpolicy/policy_parse.y
> @@ -890,10 +890,22 @@ filename : FILENAME
> { yytext[strlen(yytext) - 1] = '\0'; if (insert_id(yytext + 1,0)) return -1; }
> ;
> number : NUMBER
> - { $$ = strtoul(yytext,NULL,0); }
> + { unsigned long x;
> + errno = 0;
> + x = strtoul(yytext, NULL, 0);
> + if (errno || x > UINT_MAX)
> + return -1;
Some compilers will emit a warning if unsigned long is 32 bits. To
prevent this use:
if (errno)
return -1;
#if ULONG_MAX > UINT_MAX
if (val > UINT_MAX) {
return -1;
}
#endif
See commit b7ea65f547c67bfbae4ae133052583b090747e5a
And discussion:
https://lore.kernel.org/selinux/CAFftDdrGoQezmVSOnrFrPKaOnS3pejQXzYpfjwQ+QBHH_Pv02w@mail.gmail.com/
Jim
> + $$ = (unsigned int) x;
> + }
> ;
> number64 : NUMBER
> - { $$ = strtoull(yytext,NULL,0); }
> + { unsigned long long x;
> + errno = 0;
> + x = strtoull(yytext, NULL, 0);
> + if (errno)
> + return -1;
> + $$ = (uint64_t) x;
> + }
> ;
> ipv6_addr : IPV6_ADDR
> { if (insert_id(yytext,0)) return -1; }
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 11/13] checkpolicy: error out on parsing too big integers
2021-09-14 12:48 ` [PATCH 11/13] checkpolicy: error out on parsing too big integers Christian Göttsche
2021-09-14 20:43 ` James Carter
@ 2021-09-15 13:11 ` Christian Göttsche
1 sibling, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-15 13:11 UTC (permalink / raw)
To: selinux
Error out instead of silently converting too big integer values in
policy sources.
policy_parse.y:893:41: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
- only check usigned long against UINT_MAX, if ULONG_MAX is actually
bigger
checkpolicy/policy_parse.y | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/checkpolicy/policy_parse.y b/checkpolicy/policy_parse.y
index 6098eb50..45f973ff 100644
--- a/checkpolicy/policy_parse.y
+++ b/checkpolicy/policy_parse.y
@@ -890,10 +890,26 @@ filename : FILENAME
{ yytext[strlen(yytext) - 1] = '\0'; if (insert_id(yytext + 1,0)) return -1; }
;
number : NUMBER
- { $$ = strtoul(yytext,NULL,0); }
+ { unsigned long x;
+ errno = 0;
+ x = strtoul(yytext, NULL, 0);
+ if (errno)
+ return -1;
+#if ULONG_MAX > UINT_MAX
+ if (x > UINT_MAX)
+ return -1;
+#endif
+ $$ = (unsigned int) x;
+ }
;
number64 : NUMBER
- { $$ = strtoull(yytext,NULL,0); }
+ { unsigned long long x;
+ errno = 0;
+ x = strtoull(yytext, NULL, 0);
+ if (errno)
+ return -1;
+ $$ = (uint64_t) x;
+ }
;
ipv6_addr : IPV6_ADDR
{ if (insert_id(yytext,0)) return -1; }
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 12/13] checkpolicy: print warning on source line overflow
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (10 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 11/13] checkpolicy: error out on parsing too big integers Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-14 12:48 ` [PATCH 13/13] checkpolicy: free extended permission memory Christian Göttsche
2021-09-15 14:59 ` [PATCH 00/13] checkpolicy improvements James Carter
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux
In case the source line value overflows or has a too big value in the
source policy print a warning.
policy_scan.l:273:19: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551614 (64-bit, unsigned)
policy_scan.l:66:20: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/policy_scan.l | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/checkpolicy/policy_scan.l b/checkpolicy/policy_scan.l
index 4067268b..129a8a2a 100644
--- a/checkpolicy/policy_scan.l
+++ b/checkpolicy/policy_scan.l
@@ -59,12 +59,17 @@ alnum [a-zA-Z0-9]
hexval [0-9A-Fa-f]
%%
-\n.* { strncpy(linebuf[lno], yytext+1, 255);
- linebuf[lno][254] = 0;
- lno = 1 - lno;
- policydb_lineno++;
- source_lineno++;
- yyless(1); }
+\n.* {
+ strncpy(linebuf[lno], yytext+1, 255);
+ linebuf[lno][254] = 0;
+ lno = 1 - lno;
+ policydb_lineno++;
+ if (source_lineno == ULONG_MAX)
+ yywarn("source line number overflow");
+ else
+ source_lineno++;
+ yyless(1);
+ }
CLONE |
clone { return(CLONE); }
COMMON |
@@ -270,7 +275,13 @@ GLBLUB { return(GLBLUB); }
{hexval}{0,4}":"{hexval}{0,4}":"({hexval}|[:.])* { return(IPV6_ADDR); }
{digit}+(\.({alnum}|[_.])*)? { return(VERSION_IDENTIFIER); }
#line[ ]1[ ]\"[^\n]*\" { set_source_file(yytext+9); }
-#line[ ]{digit}+ { source_lineno = atoi(yytext+6)-1; }
+#line[ ]{digit}+ {
+ errno = 0;
+ source_lineno = strtoul(yytext+6, NULL, 10) - 1;
+ if (errno) {
+ yywarn("source line number too big");
+ }
+ }
#[^\n]* { /* delete comments */ }
[ \t\f]+ { /* delete whitespace */ }
"==" { return(EQUALS); }
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 13/13] checkpolicy: free extended permission memory
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (11 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 12/13] checkpolicy: print warning on source line overflow Christian Göttsche
@ 2021-09-14 12:48 ` Christian Göttsche
2021-09-15 14:59 ` [PATCH 00/13] checkpolicy improvements James Carter
13 siblings, 0 replies; 22+ messages in thread
From: Christian Göttsche @ 2021-09-14 12:48 UTC (permalink / raw)
To: selinux; +Cc: liwugang
define_te_avtab_xperms_helper() allocates memory for the avrule, while
define_te_avtab_ioctl() does not transfer any ownership of it.
Free the affected memory.
Direct leak of 272 byte(s) in 2 object(s) allocated from:
#0 0x49bb8d in __interceptor_malloc (./checkpolicy/checkmodule+0x49bb8d)
#1 0x4f379c in define_te_avtab_xperms_helper ./checkpolicy/policy_define.c:2047:24
#2 0x4f379c in define_te_avtab_extended_perms ./checkpolicy/policy_define.c:2469:6
#3 0x4cf417 in yyparse ./checkpolicy/policy_parse.y:494:30
#4 0x4eaf35 in read_source_policy ./checkpolicy/parse_util.c:63:6
#5 0x50cccd in main ./checkpolicy/checkmodule.c:278:7
#6 0x7fbfa455ce49 in __libc_start_main csu/../csu/libc-start.c:314:16
Direct leak of 32 byte(s) in 2 object(s) allocated from:
#0 0x49bb8d in __interceptor_malloc (./checkpolicy/checkmodule+0x49bb8d)
#1 0x4f4a38 in avrule_sort_ioctls ./checkpolicy/policy_define.c:1844:12
#2 0x4f4a38 in avrule_ioctl_ranges ./checkpolicy/policy_define.c:2021:6
#3 0x4f4a38 in define_te_avtab_ioctl ./checkpolicy/policy_define.c:2399:6
#4 0x4f4a38 in define_te_avtab_extended_perms ./checkpolicy/policy_define.c:2475:7
#5 0x4cf417 in yyparse ./checkpolicy/policy_parse.y:494:30
#6 0x4eaf35 in read_source_policy ./checkpolicy/parse_util.c:63:6
#7 0x50cccd in main ./checkpolicy/checkmodule.c:278:7
#8 0x7fbfa455ce49 in __libc_start_main csu/../csu/libc-start.c:314:16
Reported-by: liwugang <liwugang@163.com>
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
checkpolicy/policy_define.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index c71e0571..185d5704 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -2390,7 +2390,7 @@ static int avrule_cpy(avrule_t *dest, const avrule_t *src)
static int define_te_avtab_ioctl(const avrule_t *avrule_template)
{
avrule_t *avrule;
- struct av_ioctl_range_list *rangelist;
+ struct av_ioctl_range_list *rangelist, *r;
av_extended_perms_t *complete_driver, *partial_driver, *xperms;
unsigned int i;
@@ -2448,6 +2448,12 @@ done:
if (partial_driver)
free(partial_driver);
+ while (rangelist != NULL) {
+ r = rangelist;
+ rangelist = rangelist->next;
+ free(r);
+ }
+
return 0;
}
@@ -2456,6 +2462,7 @@ int define_te_avtab_extended_perms(int which)
char *id;
unsigned int i;
avrule_t *avrule_template;
+ int rc = 0;
if (pass == 1) {
for (i = 0; i < 4; i++) {
@@ -2471,15 +2478,17 @@ int define_te_avtab_extended_perms(int which)
id = queue_remove(id_queue);
if (strcmp(id,"ioctl") == 0) {
- free(id);
- if (define_te_avtab_ioctl(avrule_template))
- return -1;
+ rc = define_te_avtab_ioctl(avrule_template);
} else {
yyerror("only ioctl extended permissions are supported");
- free(id);
- return -1;
+ rc = -1;
}
- return 0;
+
+ free(id);
+ avrule_destroy(avrule_template);
+ free(avrule_template);
+
+ return rc;
}
static int define_te_avtab_helper(int which, avrule_t ** rule)
--
2.33.0
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 00/13] checkpolicy improvements
2021-09-14 12:48 [PATCH 00/13] checkpolicy improvements Christian Göttsche
` (12 preceding siblings ...)
2021-09-14 12:48 ` [PATCH 13/13] checkpolicy: free extended permission memory Christian Göttsche
@ 2021-09-15 14:59 ` James Carter
2021-09-16 20:34 ` James Carter
13 siblings, 1 reply; 22+ messages in thread
From: James Carter @ 2021-09-15 14:59 UTC (permalink / raw)
To: Christian Göttsche; +Cc: SElinux list
On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Miscellaneous improvements to the checkpolicy subproject, affecting the
> traditional language compilers checkmodule(8) and checkpolicy(8).
>
> Avoid implicit conversions, free leaked memory, resolve several compiler
> warnings, use strict integer parsing.
>
> The last patch is an adoption of a patch proposed by liwugang [1], with
> the requested changes integrated.
>
>
> [1]: https://patchwork.kernel.org/project/selinux/patch/20210601151704.2688389-1-liwugang@163.com/
>
> Christian Göttsche (13):
> libsepol: avoid implicit conversions
> libsepol: free memory after policy validation
> checkpolicy: enclose macro argument in parentheses
> checkpolicy: misc checkmodule tweaks
> checkpolicy: misc checkpolicy tweaks
> checkpolicy: mark read-only parameters in module compiler const
> checkpolicy: mark file local functions in policy_define static
> checkpolicy: add missing function declarations
> checkpolicy: resolve dismod memory leaks
> checkpolicy: avoid implicit conversion
> checkpolicy: error out on parsing too big integers
> checkpolicy: print warning on source line overflow
> checkpolicy: free extended permission memory
>
> checkpolicy/checkmodule.c | 18 +++----
> checkpolicy/checkpolicy.c | 26 +++++-----
> checkpolicy/module_compiler.c | 26 +++++-----
> checkpolicy/module_compiler.h | 4 +-
> checkpolicy/policy_define.c | 87 ++++++++++++++++----------------
> checkpolicy/policy_parse.y | 16 +++++-
> checkpolicy/policy_scan.l | 25 ++++++---
> checkpolicy/test/dismod.c | 7 ++-
> libsepol/src/policydb_validate.c | 14 ++++-
> libsepol/src/util.c | 2 +-
> 10 files changed, 131 insertions(+), 94 deletions(-)
>
> --
> 2.33.0
>
For the whole series with the v2 and v3 patches:
Acked-by: James Carter <jwcart2@gmail.com>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 00/13] checkpolicy improvements
2021-09-15 14:59 ` [PATCH 00/13] checkpolicy improvements James Carter
@ 2021-09-16 20:34 ` James Carter
0 siblings, 0 replies; 22+ messages in thread
From: James Carter @ 2021-09-16 20:34 UTC (permalink / raw)
To: Christian Göttsche; +Cc: SElinux list
On Wed, Sep 15, 2021 at 10:59 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Miscellaneous improvements to the checkpolicy subproject, affecting the
> > traditional language compilers checkmodule(8) and checkpolicy(8).
> >
> > Avoid implicit conversions, free leaked memory, resolve several compiler
> > warnings, use strict integer parsing.
> >
> > The last patch is an adoption of a patch proposed by liwugang [1], with
> > the requested changes integrated.
> >
> >
> > [1]: https://patchwork.kernel.org/project/selinux/patch/20210601151704.2688389-1-liwugang@163.com/
> >
> > Christian Göttsche (13):
> > libsepol: avoid implicit conversions
> > libsepol: free memory after policy validation
> > checkpolicy: enclose macro argument in parentheses
> > checkpolicy: misc checkmodule tweaks
> > checkpolicy: misc checkpolicy tweaks
> > checkpolicy: mark read-only parameters in module compiler const
> > checkpolicy: mark file local functions in policy_define static
> > checkpolicy: add missing function declarations
> > checkpolicy: resolve dismod memory leaks
> > checkpolicy: avoid implicit conversion
> > checkpolicy: error out on parsing too big integers
> > checkpolicy: print warning on source line overflow
> > checkpolicy: free extended permission memory
> >
> > checkpolicy/checkmodule.c | 18 +++----
> > checkpolicy/checkpolicy.c | 26 +++++-----
> > checkpolicy/module_compiler.c | 26 +++++-----
> > checkpolicy/module_compiler.h | 4 +-
> > checkpolicy/policy_define.c | 87 ++++++++++++++++----------------
> > checkpolicy/policy_parse.y | 16 +++++-
> > checkpolicy/policy_scan.l | 25 ++++++---
> > checkpolicy/test/dismod.c | 7 ++-
> > libsepol/src/policydb_validate.c | 14 ++++-
> > libsepol/src/util.c | 2 +-
> > 10 files changed, 131 insertions(+), 94 deletions(-)
> >
> > --
> > 2.33.0
> >
>
> For the whole series with the v2 and v3 patches:
>
> Acked-by: James Carter <jwcart2@gmail.com>
This series has been merged.
Thanks,
Jim
^ permalink raw reply [flat|nested] 22+ messages in thread