On Thu, Apr 29, 2021 at 9:25 PM Gedare Bloom <ged...@rtems.org> wrote:
> On Wed, Apr 28, 2021 at 9:04 PM Niteesh G. S. <niteesh...@gmail.com> > wrote: > > > > > > > > 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) > 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@rtems.org > >> > http://lists.rtems.org/mailman/listinfo/devel >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel