On Sat, Oct 3, 2020 at 3:40 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
>
> Contrary to Linux kernel, BUG_ON() does not halt the execution, in
> libsepol/src/services.c. Instead it displays an error message and
> continues the execution.
>
> This means that this code does not prevent an out-of-bound write from
> happening:
>
> case CEXPR_AND:
> BUG_ON(sp < 1);
> sp--;
> s[sp] &= s[sp + 1];
>
> Use if(...){BUG();rc=-EINVAL;goto out;} constructions instead, to make
> sure that the array access is always in-bound.
>
> This issue has been found using clang's static analyzer:
> https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-50a861.html#EndPath
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
Acked-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> libsepol/src/services.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> index 90da1f4efef3..beb0711f6680 100644
> --- a/libsepol/src/services.c
> +++ b/libsepol/src/services.c
> @@ -67,7 +67,6 @@
> #include "flask.h"
>
> #define BUG() do { ERR(NULL, "Badness at %s:%d", __FILE__, __LINE__); } while (0)
> -#define BUG_ON(x) do { if (x) ERR(NULL, "Badness at %s:%d", __FILE__, __LINE__); } while (0)
>
> static int selinux_enforcing = 1;
>
> @@ -469,18 +468,30 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
> /* Now process each expression of the constraint */
> switch (e->expr_type) {
> case CEXPR_NOT:
> - BUG_ON(sp < 0);
> + if (sp < 0) {
> + BUG();
> + rc = -EINVAL;
> + goto out;
> + }
> s[sp] = !s[sp];
> cat_expr_buf(expr_list[expr_counter], "not");
> break;
> case CEXPR_AND:
> - BUG_ON(sp < 1);
> + if (sp < 1) {
> + BUG();
> + rc = -EINVAL;
> + goto out;
> + }
> sp--;
> s[sp] &= s[sp + 1];
> cat_expr_buf(expr_list[expr_counter], "and");
> break;
> case CEXPR_OR:
> - BUG_ON(sp < 1);
> + if (sp < 1) {
> + BUG();
> + rc = -EINVAL;
> + goto out;
> + }
> sp--;
> s[sp] |= s[sp + 1];
> cat_expr_buf(expr_list[expr_counter], "or");
> --
> 2.28.0
>
