| Message ID | 20201003193957.1876526-2-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:41 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > When find_avtab_node() is called with key->specified & AVTAB_XPERMS and > xperms=NULL, xperms is being dereferenced. This is detected as a > "NULL pointer dereference issue" by static analyzers. > > Even though it does not make much sense to call find_avtab_node() in a > way which triggers the NULL pointer dereference issue, static analyzers > have a hard time with calls such as: > > node = find_avtab_node(handle, avtab, &avkey, cond, NULL); > > ... where xperms=NULL. > > So, make the function report an error instead of crashing. > > Here is an example of report from clang's static analyzer: > https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> Acked-by: James Carter <jwcart2@gmail.com> > --- > libsepol/src/expand.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > index 19e48c507236..eac7e4507d02 100644 > --- a/libsepol/src/expand.c > +++ b/libsepol/src/expand.c > @@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > > /* AVTAB_XPERMS entries are not necessarily unique */ > if (key->specified & AVTAB_XPERMS) { > - node = avtab_search_node(avtab, key); > - while (node) { > - if ((node->datum.xperms->specified == xperms->specified) && > - (node->datum.xperms->driver == xperms->driver)) { > - match = 1; > - break; > + if (xperms == NULL) { > + ERR(handle, "searching xperms NULL"); > + node = NULL; > + } else { > + node = avtab_search_node(avtab, key); > + while (node) { > + if ((node->datum.xperms->specified == xperms->specified) && > + (node->datum.xperms->driver == xperms->driver)) { > + match = 1; > + break; > + } > + node = avtab_search_node_next(node, key->specified); > } > - node = avtab_search_node_next(node, key->specified); > + if (!match) > + node = NULL; > } > - if (!match) > - node = NULL; > } else { > node = avtab_search_node(avtab, key); > } > -- > 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:41 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote: > > > > When find_avtab_node() is called with key->specified & AVTAB_XPERMS and > > xperms=NULL, xperms is being dereferenced. This is detected as a > > "NULL pointer dereference issue" by static analyzers. > > > > Even though it does not make much sense to call find_avtab_node() in a > > way which triggers the NULL pointer dereference issue, static analyzers > > have a hard time with calls such as: > > > > node = find_avtab_node(handle, avtab, &avkey, cond, NULL); > > > > ... where xperms=NULL. > > > > So, make the function report an error instead of crashing. > > > > Here is an example of report from clang's static analyzer: > > https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath > > > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> > > Acked-by: James Carter <jwcart2@gmail.com> Thanks. Merged. Nicolas > > --- > > libsepol/src/expand.c | 23 ++++++++++++++--------- > > 1 file changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c > > index 19e48c507236..eac7e4507d02 100644 > > --- a/libsepol/src/expand.c > > +++ b/libsepol/src/expand.c > > @@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, > > > > /* AVTAB_XPERMS entries are not necessarily unique */ > > if (key->specified & AVTAB_XPERMS) { > > - node = avtab_search_node(avtab, key); > > - while (node) { > > - if ((node->datum.xperms->specified == xperms->specified) && > > - (node->datum.xperms->driver == xperms->driver)) { > > - match = 1; > > - break; > > + if (xperms == NULL) { > > + ERR(handle, "searching xperms NULL"); > > + node = NULL; > > + } else { > > + node = avtab_search_node(avtab, key); > > + while (node) { > > + if ((node->datum.xperms->specified == xperms->specified) && > > + (node->datum.xperms->driver == xperms->driver)) { > > + match = 1; > > + break; > > + } > > + node = avtab_search_node_next(node, key->specified); > > } > > - node = avtab_search_node_next(node, key->specified); > > + if (!match) > > + node = NULL; > > } > > - if (!match) > > - node = NULL; > > } else { > > node = avtab_search_node(avtab, key); > > } > > -- > > 2.28.0 > >
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c index 19e48c507236..eac7e4507d02 100644 --- a/libsepol/src/expand.c +++ b/libsepol/src/expand.c @@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle, /* AVTAB_XPERMS entries are not necessarily unique */ if (key->specified & AVTAB_XPERMS) { - node = avtab_search_node(avtab, key); - while (node) { - if ((node->datum.xperms->specified == xperms->specified) && - (node->datum.xperms->driver == xperms->driver)) { - match = 1; - break; + if (xperms == NULL) { + ERR(handle, "searching xperms NULL"); + node = NULL; + } else { + node = avtab_search_node(avtab, key); + while (node) { + if ((node->datum.xperms->specified == xperms->specified) && + (node->datum.xperms->driver == xperms->driver)) { + match = 1; + break; + } + node = avtab_search_node_next(node, key->specified); } - node = avtab_search_node_next(node, key->specified); + if (!match) + node = NULL; } - if (!match) - node = NULL; } else { node = avtab_search_node(avtab, key); }
When find_avtab_node() is called with key->specified & AVTAB_XPERMS and xperms=NULL, xperms is being dereferenced. This is detected as a "NULL pointer dereference issue" by static analyzers. Even though it does not make much sense to call find_avtab_node() in a way which triggers the NULL pointer dereference issue, static analyzers have a hard time with calls such as: node = find_avtab_node(handle, avtab, &avkey, cond, NULL); ... where xperms=NULL. So, make the function report an error instead of crashing. Here is an example of report from clang's static analyzer: https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org> --- libsepol/src/expand.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)