[PATCH] bsps/shared/ofw: Fix coverity defects

[Niteesh G. S.]

On Thu, Apr 29, 2021 at 12:50 AM Gedare Bloom <gedare at rtems.org> wrote:

> On Wed, Apr 28, 2021 at 11:30 AM G S Niteesh Babu <niteesh.gs at gmail.com>
> wrote:
> >
> > This patch adds asserts to fix coverity defects
> > 1) CID 1474437 (Out-of-bounds access)
> > 2) CID 1474436 (Out-of-bounds access)
> >
> > From manual inspection, out of bounds access cannot occur due to
> > bounds checking but coverity fails to detect the checks.
> > We are adding asserts as a secondary check.
> > ---
> >  bsps/shared/ofw/ofw.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/bsps/shared/ofw/ofw.c b/bsps/shared/ofw/ofw.c
> > index f4b8b63931..808fa85d81 100644
> > --- a/bsps/shared/ofw/ofw.c
> > +++ b/bsps/shared/ofw/ofw.c
> > @@ -42,6 +42,7 @@
> >  #include <assert.h>
> >  #include <rtems/sysinit.h>
> >  #include <ofw/ofw_test.h>
> > +#include <rtems/score/assert.h>
> >
> >  static void *fdtp = NULL;
> >
> > @@ -186,6 +187,7 @@ ssize_t rtems_ofw_get_prop(
> >    const void *prop;
> >    int offset;
> >    int len;
> > +  int copy_len;
> >    uint32_t cpuid;
> >
> >    offset = rtems_fdt_phandle_to_offset(node);
> > @@ -226,7 +228,9 @@ ssize_t rtems_ofw_get_prop(
> >      return -1;
> >    }
> >
> > -  bcopy(prop, buf, MIN(len, bufsize));
> > +  copy_len = MIN(len, bufsize);
> > +  _Assert(copy_len <= bufsize);
> > +  memmove(prop, buf, copy_len);
> >
> >    return len;
> >  }
> > @@ -637,6 +641,12 @@ int rtems_ofw_get_reg(
> >          range.child_bus = fdt32_to_cpu(ptr[j].child_bus);
> >          range.size = fdt32_to_cpu(ptr[j].size);
> >
> > +        /*
> > +         * buf[i + 1] should upperbound the access for buf[i].
> > +         * Thus by making sure buf[i + 1] <= (buf + size) we
> > +         * can be sure buf[i] will always be inbounds.
> > +         */
> > +        _Assert(buf[i + 1] <= (buf + size));
> This looks suspect. It can make an out-of-bounds read access I think. How
> about
> _Assert(i < size);
> Is that equivalent?
>
No that won't work because size is the length of the buffer in bytes. I
right thing would be
_Assert(i < nregs) where nregs = size / sizeof(rtems_ofw_reg)
but I don't think adding this will make any change.
BTW what makes you suspect that it can still make an out-of-bounds access?

>
> >          if (buf[i].start >= range.child_bus &&
> >              buf[i].start < range.child_bus + range.size) {
> >            offset = range.parent_bus - range.child_bus;
> > --
> > 2.17.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210429/4cec31cc/attachment-0001.html>