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

[Gedare Bloom]

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).

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).

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