On Tue, Mar 16, 2021 at 5:45 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
>
> 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;
>
This looks good to me.
> ... 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?
>
uint32_t is probably used because both startbit in an ebitmap_node_t
and highbit in an ebitmap_t are uint32_t. Although the ebitmap
functions take unsigned int for bit position and return unsigned int
for bit position as well.
I do agree that it would be better to make the type consistent, but,
since num_names has type int in many places, I would rather change it
everywhere to unsigned int. I'll send out a patch.
> 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)
>
I don't know about compatibility issues, but I would prefer to use the
builtins as long as they won't cause problems.
Jim
> Cheers,
> Nicolas
>
