<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 Fri, Feb 5, 2021 at 12:22 AM Christian Mauderer <<a href="mailto:contact@c-mauderer.de">contact@c-mauderer.de</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"><br>
<br>
On 04/02/2021 17:34, Gedare Bloom wrote:<br>
> <br>
> <br>
> On Thu, Feb 4, 2021 at 1:58 AM Niteesh G. S. <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a> <br>
> <mailto:<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>>> wrote:<br>
> <br>
> <br>
> <br>
> On Thu, Feb 4, 2021 at 1:21 AM Gedare Bloom <<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a><br>
> <mailto:<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>>> wrote:<br>
> <br>
> <br>
> <br>
> On Wed, Feb 3, 2021 at 2:48 AM G S Niteesh Babu<br>
> <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a> <mailto:<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>>> wrote:<br>
> <br>
> Fixed use after free and null pointer dereference defects<br>
> <br>
> FIXES:<br>
> 1) CID 1472601 (NULL_RETURNS)<br>
> 2) CID 1472600 (USE_AFTER_FREE)<br>
> 3) CID 1472599 (USE_AFTER_FREE)<br>
> 4) CID 1472598 (USE_AFTER_FREE)<br>
> 5) CID 1472596 (USE_AFTER_FREE)<br>
> 6) CID 1472597 (ARRAY_VS_SINGLETON)<br>
> 7) CID 1472595 (ARRAY_VS_SINGLETON)<br>
> ---<br>
> bsps/shared/ofw/ofw.c | 36<br>
> ++++++++++++++++++------------------<br>
> 1 file changed, 18 insertions(+), 18 deletions(-)<br>
> <br>
> diff --git a/bsps/shared/ofw/ofw.c b/bsps/shared/ofw/ofw.c<br>
> index 82924b2600..ccd57e36af 100644<br>
> --- a/bsps/shared/ofw/ofw.c<br>
> +++ b/bsps/shared/ofw/ofw.c<br>
> @@ -313,7 +313,7 @@ ssize_t rtems_ofw_get_prop_alloc(<br>
> }<br>
> <br>
> if (rtems_ofw_get_prop(node, propname, *buf, len) == -1) {<br>
> - rtems_ofw_free(buf);<br>
> + rtems_ofw_free(*buf);<br>
> *buf = NULL;<br>
> return -1;<br>
> }<br>
> @@ -344,7 +344,7 @@ ssize_t rtems_ofw_get_prop_alloc_multi(<br>
> }<br>
> <br>
> if (rtems_ofw_get_prop(node, propname, *buf, len) == -1) {<br>
> - rtems_ofw_free(buf);<br>
> + rtems_ofw_free(*buf);<br>
> *buf = NULL;<br>
> return -1;<br>
> }<br>
> @@ -373,7 +373,7 @@ ssize_t rtems_ofw_get_enc_prop_alloc(<br>
> }<br>
> <br>
> if (rtems_ofw_get_enc_prop(node, propname, *buf, len)<br>
> == -1) {<br>
> - rtems_ofw_free(buf);<br>
> + rtems_ofw_free(*buf);<br>
> *buf = NULL;<br>
> return -1;<br>
> }<br>
> @@ -404,7 +404,7 @@ ssize_t rtems_ofw_get_enc_prop_alloc_multi(<br>
> }<br>
> <br>
> if (rtems_ofw_get_enc_prop(node, propname, *buf, len)<br>
> == -1) {<br>
> - rtems_ofw_free(buf);<br>
> + rtems_ofw_free(*buf);<br>
> *buf = NULL;<br>
> return -1;<br>
> }<br>
> <br>
> The above all look fine to me.<br>
> <br>
> @@ -500,21 +500,21 @@ static phandle_t<br>
> rtems_ofw_get_effective_phandle(<br>
> )<br>
> {<br>
> phandle_t child;<br>
> - phandle_t ref;<br>
> + phandle_t ref[1];<br>
> <br>
> <br>
> I don't like this. I know this was suggested, but I think it is<br>
> a little ridiculous. This is a false positive since we know that<br>
> sizeof(*buf) == len. I don't know if we can make that an<br>
> assertion. Otherwise, we can just mark this as a false positive<br>
> in coverity. We know the array dereference in this case won't<br>
> overwrite past the bounds of ref.<br>
> <br>
> Instead of using hard-coded values of 4<br>
> in rtems_ofw_get_enc_prop() you might make it more explicitly<br>
> using sizeof(pcell_t), since that is what you mean.<br>
> <br>
> Done.<br>
> <br>
> <br>
> I would also agree to change the strncpy as Christian identified<br>
> before in rtems_ofw_get_prop().<br>
> <br>
> Is the reason to avoid strncpy that it ignores the null byte if<br>
> len(dst) <= len(src)?<br>
> If so can I do an explicit null byte append?<br>
> Or is there any other reason?<br>
> <br>
> The reason is that it passes void* pointers. If these are strings, you <br>
> should use char* type. Otherwise, memcpy is more suitable.<br>
> <br>
> It also would be generally safer to overwrite with the NIL character to <br>
> guarantee it is a null-terminated string, if that is expected.<br>
<br>
That was not the only reason. Let me pull the relevant lines together <br>
(reordered and pulled from multiple files):<br>
<br>
<br>
typedef uint32_t pcell_t;<br>
<br>
rtems_ofw_get_enc_prop(<br>
phandle_t node,<br>
const char *prop,<br>
pcell_t *buf,<br>
size_t len<br>
)<br>
{<br>
...<br>
rv = rtems_ofw_get_prop(node, prop, buf, len);<br>
...<br>
}<br>
<br>
rtems_ofw_get_prop(<br>
phandle_t node,<br>
const char *propname,<br>
void *buf,<br>
size_t bufsize<br>
{<br>
...<br>
strncpy(buf, prop, bufsize);<br>
...<br>
}<br>
<br>
Let's say I do the following call:<br>
<br>
rtems_ofw_get_enc_prop(node, "name", &foo, sizeof(foo));<br>
<br>
In that case the code is using a strncpy to copy into a uint32_t. That's <br>
not a good idea. What if there is (for example) a value of 0x00110011 in <br>
the property? strncpy will find one of these two 0 bytes and stop there. <br>
I'm not sure which one because endianess will have an influence on that <br>
too. Note that I'm not sure whether using rtems_ofw_get_enc_prob with <br>
these parameters is a useful call. But it's possible and it's a bad idea <br>
if it results in an undefined behavior.</blockquote><div><div class="gmail_default" style="font-size:small">OK.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Summarizing changes since the last posted patch (v3)</div><div class="gmail_default" style="font-size:small">1) Using a variable instead of an array for CID 1472597, 1472595</div><div class="gmail_default" style="font-size:small"> so this has to be marked as a false positive in Coverity since we</div><div class="gmail_default" style="font-size:small"> guarantee that we don't overflow.</div><div class="gmail_default" style="font-size:small">2) rtems_ofw_get_effective_phandle is now iterative instead of recursive.</div><div class="gmail_default" style="font-size:small">3) Using memcpy instead of strncpy</div><div class="gmail_default" style="font-size:small">4) I have also fixed another bug, should I post it as part of this patch or a</div><div class="gmail_default" style="font-size:small">separate patch?</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Thanks,</div><div class="gmail_default" style="font-size:small">Niteesh.</div><div class="gmail_default" style="font-size:small"> <br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Best regards<br>
<br>
Christian<br>
<br>
> <br>
> <br>
> for (child = rtems_ofw_child(node); child != 0; child =<br>
> rtems_ofw_peer(child)) {<br>
> - ref = rtems_ofw_get_effective_phandle(child, xref);<br>
> - if (ref != -1)<br>
> - return ref;<br>
> + ref[0] = rtems_ofw_get_effective_phandle(child, xref);<br>
> <br>
> <br>
> I didn't notice before, but is this recursion bounded (yes, it<br>
> is a tree, but it might be better to rewrite this function<br>
> non-recursively).<br>
> <br>
> Just curious why is it better? Is it because it might use a lot of<br>
> stack frames?<br>
> I can only think of using stack or queue to implement it<br>
> non-recursively. Is there<br>
> any other way?<br>
> <br>
> <br>
> Recursion causes two potential problems: large stack usage and <br>
> hard-to-analyze execution times. These are generally important for an <br>
> RTOS to be wary of.<br>
> <br>
> This looks like a depth-first search to find xref? But the tree <br>
> traversal order doesn't matter. In fact, I would check if the FDT can be <br>
> iterated directly. I don't know enough about the FDT structure to say <br>
> whether that is simple to do. If you start at the root and repeatedly <br>
> call fdt_next_node() do you traverse all the nodes?<br>
> <br>
> You can implement non-recursive tree searches using nested loops when <br>
> you have sibling, child, and parent pointers. Probably, you can find <br>
> code examples of how to do this. The basic idea is pretty simple though, <br>
> here is a DFS:<br>
> node = root<br>
> do {<br>
> visit(node)<br>
> next_node = child_of(node)<br>
> if ( ! next_node ) {<br>
> while ( !has_sibling(node) && node != root) {<br>
> node = parent_of(node) /* back up */<br>
> }<br>
> next_node = sibling_of(node)<br>
> }<br>
> node = next_node;<br>
> } while (node)<br>
> <br>
> This pseudocode assumes the root has a NULL-value sibling and leaves <br>
> have NULL-value children. I also didn't test it, but the rough idea <br>
> should work. You can do something similar with BFS.<br>
> <br>
> + if (ref[0] != -1)<br>
> + return ref[0];<br>
> <br>
> - if (rtems_ofw_get_enc_prop(child, "phandle", &ref,<br>
> sizeof(ref)) == -1 &&<br>
> - rtems_ofw_get_enc_prop(child, "ibm,phandle", &ref,<br>
> sizeof(ref)) == -1 &&<br>
> - rtems_ofw_get_enc_prop(child, "linux,phandle",<br>
> &ref, sizeof(ref)) == -1<br>
> + if (rtems_ofw_get_enc_prop(child, "phandle", ref,<br>
> sizeof(ref)) == -1 &&<br>
> + rtems_ofw_get_enc_prop(child, "ibm,phandle", ref,<br>
> sizeof(ref)) == -1 &&<br>
> + rtems_ofw_get_enc_prop(child, "linux,phandle", ref,<br>
> sizeof(ref)) == -1<br>
> ) {<br>
> continue;<br>
> }<br>
> <br>
> - if (ref == xref)<br>
> + if (ref[0] == xref)<br>
> return child;<br>
> }<br>
> <br>
> @@ -533,16 +533,16 @@ phandle_t rtems_ofw_node_from_xref(<br>
> phandle_t xref )<br>
> <br>
> phandle_t rtems_ofw_xref_from_node( phandle_t node )<br>
> {<br>
> - phandle_t ref;<br>
> + phandle_t ref[1];<br>
> <br>
> - if (rtems_ofw_get_enc_prop(node, "phandle", &ref,<br>
> sizeof(ref)) == -1 &&<br>
> - rtems_ofw_get_enc_prop(node, "ibm,phandle", &ref,<br>
> sizeof(ref)) == -1 &&<br>
> - rtems_ofw_get_enc_prop(node, "linux,phandle", &ref,<br>
> sizeof(ref)) == -1)<br>
> + if (rtems_ofw_get_enc_prop(node, "phandle", ref,<br>
> sizeof(ref)) == -1 &&<br>
> + rtems_ofw_get_enc_prop(node, "ibm,phandle", ref,<br>
> sizeof(ref)) == -1 &&<br>
> + rtems_ofw_get_enc_prop(node, "linux,phandle", ref,<br>
> sizeof(ref)) == -1)<br>
> {<br>
> return node;<br>
> }<br>
> <br>
> - return ref;<br>
> + return ref[0];<br>
> }<br>
> <br>
> phandle_t rtems_ofw_instance_to_package( ihandle_t instance )<br>
> @@ -614,7 +614,7 @@ int rtems_ofw_get_reg(<br>
> offset = rtems_fdt_phandle_to_offset(parent);<br>
> ptr = fdt_getprop(fdtp, offset, "ranges", &len);<br>
> <br>
> - if (len < 0) {<br>
> + if (ptr == NULL) {<br>
> break;<br>
> }<br>
> <br>
> -- <br>
> 2.17.1<br>
> <br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a> <mailto:<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>
> <<a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a>><br>
> <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>
> <br>
</blockquote></div></div>