diff mbox series

[1/2] libsepol: drop confusing BUG_ON macro

Message ID 20201003193957.1876526-1-nicolas.iooss@m4x.org
State Accepted
Headers show
Series [1/2] libsepol: drop confusing BUG_ON macro | expand

Commit Message

Nicolas Iooss Oct. 3, 2020, 7:39 p.m. UTC
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@m4x.org>
---
 libsepol/src/services.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

Comments

James Carter Oct. 15, 2020, 5 p.m. UTC | #1
On Sat, Oct 3, 2020 at 3:40 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  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
>
Nicolas Iooss Oct. 19, 2020, 9:28 p.m. UTC | #2
On Thu, Oct 15, 2020 at 7:00 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sat, Oct 3, 2020 at 3:40 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>

Thanks. Merged.

Nicolas
>
> > ---
> >  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
> >
diff mbox series

Patch

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");