On Thu, Apr 29, 2021 at 12:50 AM Gedare Bloom <ged...@rtems.org> wrote:
> On Wed, Apr 28, 2021 at 11:30 AM G S Niteesh Babu <niteesh...@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@rtems.org > > http://lists.rtems.org/mailman/listinfo/devel >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel