| Message ID | 20201003193957.1876526-1-nicolas.iooss@m4x.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] libsepol: drop confusing BUG_ON macro | expand |
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 >
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 --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");
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(-)