<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 9:25 PM 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 9:04 PM Niteesh G. S. <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> 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>
>><br>
>> 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>
><br>
> No that won't work because size is the length of the buffer in bytes. I right thing would be<br>
> _Assert(i < nregs) where nregs = size / sizeof(rtems_ofw_reg)<br>
You can also do this generically with size / sizeof(buf[0]). We might<br>
have such helpers already for array / address calculations, I'm not<br>
sure.<br>
<br>
> but I don't think adding this will make any change.<br>
> BTW what makes you suspect that it can still make an out-of-bounds access?<br>
I meant that buf[i+1] is out of range if (i >= size - 1).<br></blockquote><div><span class="gmail_default" style="font-size:small">yeah, I actually meant to compare the address, so it should be (buf + i + 1)</span></div><div><span class="gmail_default" style="font-size:small">like you mentioned below</span></div><div><span class="gmail_default" style="font-size:small"></span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
However, even then the logic is suspicious, you're comparing the value<br>
at buf[i+1] to the address of buf+size. What you mean is &buf[i+1] <=<br>
(buf+size). This actually might not be an out-of-bounds access then, I<br>
think you can do this safely since you don't dereference *(buf + i +<br>
1).<br></blockquote><div><span class="gmail_default" style="font-size:small">Yes.</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>
Is it also correct to use &buf[i] < (buf+size)?</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> That will be better<br>
than testing equality. Or, use<br>
&buf[i] < (buf + size - (sizeof(buf[0]) - 1))<br></blockquote><div><span class="gmail_default" style="font-size:small">I am sending a V2 with this one.</span><span class="gmail_default"></span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
since what you really want to confirm is that buf[i] is not going to<br>
access any bytes past the buf+size.<br>
<br>
>><br>
>><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>