On Tue, Mar 16, 2021 at 9:49 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> If a role or user attribute with nothing associated with it is used
> in a constraint expression, then the bitmap will be empty. This is
> not a problem for the kernel, but does cause problems when converting
> a kernel policy or module to CIL.
>
> When creating a CIL policy from a kernel policy or module, if an
> empty bitmap is encountered, use the string "NO_IDENTIFIER". An
> error will occur if an attempt is made to compile the resulting
> policy, but a valid policy was not being produced before anyway.
> Treat types the same way even though empty bitmaps are not expected.
>
> Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> libsepol/src/kernel_to_cil.c | 2 +-
> libsepol/src/module_to_cil.c | 10 +++++++---
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/libsepol/src/kernel_to_cil.c b/libsepol/src/kernel_to_cil.c
> index 96e0f5d3..c6dd2e12 100644
> --- a/libsepol/src/kernel_to_cil.c
> +++ b/libsepol/src/kernel_to_cil.c
> @@ -189,7 +189,7 @@ static char *constraint_expr_to_str(struct policydb *pdb, struct constraint_expr
> names = ebitmap_to_str(&curr->names, pdb->p_role_val_to_name, 1);
> }
> if (!names) {
> - goto exit;
> + names = strdup("NO_IDENTIFIER");
> }
> if (strchr(names, ' ')) {
> new_val = create_str("(%s %s (%s))", 3, op, attr1, names);
> diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
> index 3cc75b42..2a794f57 100644
> --- a/libsepol/src/module_to_cil.c
> +++ b/libsepol/src/module_to_cil.c
> @@ -1793,9 +1793,13 @@ static int constraint_expr_to_string(struct policydb *pdb, struct constraint_exp
> goto exit;
> }
> }
> - rc = name_list_to_string(name_list, num_names, &names);
> - if (rc != 0) {
> - goto exit;
> + if (num_names == 0) {
> + names = strdup("NO_IDENTIFIER");
> + } else {
> + rc = name_list_to_string(name_list, num_names, &names);
> + if (rc != 0) {
> + goto exit;
> + }
> }
>
> // length of values/oper + 2 spaces + 2 parens + null terminator
Hello,
This change somehow made gcc unhappy:
$ gcc -O2 -c module_to_cil.c
In function ‘name_list_to_string’,
inlined from ‘constraint_expr_to_string’ at module_to_cil.c:1799:11:
module_to_cil.c:1156:8: warning: argument 1 range
[18446744071562067968, 18446744073709551615] exceeds maximum object
size 9223372036854775807 [-Walloc-size-larger-than=]
1156 | str = malloc(len);
| ^~~~~~~~~~~
In file included from module_to_cil.c:39:
module_to_cil.c: In function ‘constraint_expr_to_string’:
/usr/include/stdlib.h:539:14: note: in a call to allocation function
‘malloc’ declared here
539 | extern void *malloc (size_t __size) __THROW __attribute_malloc__
| ^~~~~~
(With gcc 10.2.0 on Arch Linux and gcc 9.3.0-17ubuntu1 on Ubuntu 20.04
which is used by GitHub Actions,
https://github.com/fishilico/selinux/runs/2125501324?check_suite_focus=true#step:9:107
; building for x86_64)
The main cause of this error is the fact that num_names is considered
as a signed integer in name_list_to_string(). This patch fixes the
issue:
diff --git a/libsepol/src/module_to_cil.c b/libsepol/src/module_to_cil.c
index 2a794f577841..6185c7e4ccb7 100644
--- a/libsepol/src/module_to_cil.c
+++ b/libsepol/src/module_to_cil.c
@@ -1124,7 +1124,7 @@ exit:
}
-static int name_list_to_string(char **names, int num_names, char **string)
+static int name_list_to_string(char **names, unsigned int num_names,
char **string)
{
// create a space separated string of the names
int rc = -1;
... but it would be even better if the type of num_names was
consistent. Right now, ebitmap_to_names() initializes a local variable
"uint32_t num;" and then does "*num_names = num;" with "int
*num_names". I believe the code would be more correct if the parameter
of ebitmap_to_names() was "uint32_t *num_names" or "unsigned int
*num_names" (why is uint32_t used?), and if all its callers used an
unsigned type to store this value. What do you think?
Moreover, I stumbled upon this code pattern in function name_list_to_string:
len += strlen(names[i]);
if (len < strlen(names[i])) {
log_err("Overflow");
return -1;
}
Nowadays, both gcc and clang provide checked arithmetic builtins and I
think the intent of this code would be clearer if it used them:
if (__builtin_add_overflow(len, strlen(names[i]), &len)) {
log_err("Overflow");
return -1;
}
Does anyone have an opinion about using checked arithmetic builtins?
(I have not used them much, and if someone knows of some compatibility
issues, this would be important to know)
Cheers,
Nicolas
