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

[Niteesh G. S.]

On Thu, Apr 29, 2021 at 9:25 PM Gedare Bloom <gedare at rtems.org> wrote:

> On Wed, Apr 28, 2021 at 9:04 PM Niteesh G. S. <niteesh.gs at gmail.com>
> wrote:
> >
> >
> >
> > 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)
> You can also do this generically with size / sizeof(buf[0]). We might
> have such helpers already for array / address calculations, I'm not
> sure.
>
> > 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?
> I meant that buf[i+1] is out of range if (i >= size - 1).
>
yeah, I actually meant to compare the address, so it should be (buf + i + 1)
like you mentioned below


> However, even then the logic is suspicious, you're comparing the value
> at buf[i+1] to the address of buf+size. What you mean is &buf[i+1] <=
> (buf+size). This actually might not be an out-of-bounds access then, I
> think you can do this safely since you don't dereference *(buf + i +
> 1).
>
Yes.

>
> Is it also correct to use &buf[i] < (buf+size)?

That will be better
> than testing equality. Or, use
> &buf[i] < (buf + size - (sizeof(buf[0]) - 1))
>
I am sending a V2 with this one.

> since what you really want to confirm is that buf[i] is not going to
> access any bytes past the buf+size.
>
> >>
> >>
> >> >          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/20210501/0f35c870/attachment.html>