<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Apr 29, 2021 at 12:50 AM Gedare Bloom <<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Apr 28, 2021 at 11:30 AM G S Niteesh Babu <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>> wrote:<br>
><br>
> This patch adds asserts to fix coverity defects<br>
> 1) CID 1474437 (Out-of-bounds access)<br>
> 2) CID 1474436 (Out-of-bounds access)<br>
><br>
> From manual inspection, out of bounds access cannot occur due to<br>
> bounds checking but coverity fails to detect the checks.<br>
> We are adding asserts as a secondary check.<br>
> ---<br>
>  bsps/shared/ofw/ofw.c | 12 +++++++++++-<br>
>  1 file changed, 11 insertions(+), 1 deletion(-)<br>
><br>
> diff --git a/bsps/shared/ofw/ofw.c b/bsps/shared/ofw/ofw.c<br>
> index f4b8b63931..808fa85d81 100644<br>
> --- a/bsps/shared/ofw/ofw.c<br>
> +++ b/bsps/shared/ofw/ofw.c<br>
> @@ -42,6 +42,7 @@<br>
>  #include <assert.h><br>
>  #include <rtems/sysinit.h><br>
>  #include <ofw/ofw_test.h><br>
> +#include <rtems/score/assert.h><br>
><br>
>  static void *fdtp = NULL;<br>
><br>
> @@ -186,6 +187,7 @@ ssize_t rtems_ofw_get_prop(<br>
>    const void *prop;<br>
>    int offset;<br>
>    int len;<br>
> +  int copy_len;<br>
>    uint32_t cpuid;<br>
><br>
>    offset = rtems_fdt_phandle_to_offset(node);<br>
> @@ -226,7 +228,9 @@ ssize_t rtems_ofw_get_prop(<br>
>      return -1;<br>
>    }<br>
><br>
> -  bcopy(prop, buf, MIN(len, bufsize));<br>
> +  copy_len = MIN(len, bufsize);<br>
> +  _Assert(copy_len <= bufsize);<br>
> +  memmove(prop, buf, copy_len);<br>
><br>
>    return len;<br>
>  }<br>
> @@ -637,6 +641,12 @@ int rtems_ofw_get_reg(<br>
>          range.child_bus = fdt32_to_cpu(ptr[j].child_bus);<br>
>          range.size = fdt32_to_cpu(ptr[j].size);<br>
><br>
> +        /*<br>
> +         * buf[i + 1] should upperbound the access for buf[i].<br>
> +         * Thus by making sure buf[i + 1] <= (buf + size) we<br>
> +         * can be sure buf[i] will always be inbounds.<br>
> +         */<br>
> +        _Assert(buf[i + 1] <= (buf + size));<br>
This looks suspect. It can make an out-of-bounds read access I think. How about<br>
_Assert(i < size);<br>
Is that equivalent?<br></blockquote><div><span class="gmail_default" style="font-size:small">No that won't work because size is the length of the buffer in bytes. I right thing would be</span></div><div><span class="gmail_default" style="font-size:small">_Assert(i < nregs) where nregs = size / sizeof(rtems_ofw_reg)</span></div><div><span class="gmail_default" style="font-size:small">but I don't think adding this will make any change.</span></div><div><span class="gmail_default" style="font-size:small">BTW what makes you suspect that it can still make an out-of-bounds access?</span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
>          if (buf[i].start >= range.child_bus &&<br>
>              buf[i].start < range.child_bus + range.size) {<br>
>            offset = range.parent_bus - range.child_bus;<br>
> --<br>
> 2.17.1<br>
><br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
</blockquote></div></div>