<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 4, 2021 at 1:58 AM Niteesh G. S. <<a href="mailto:niteesh.gs@gmail.com">niteesh.gs@gmail.com</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"><div dir="ltr"><div dir="ltr"><div style="font-size:small"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 4, 2021 at 1:21 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"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 3, 2021 at 2:48 AM G S Niteesh Babu <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</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">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>
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) == -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) == -1) {<br>
- rtems_ofw_free(buf);<br>
+ rtems_ofw_free(*buf);<br>
*buf = NULL;<br>
return -1;<br>
}<br></blockquote><div>The above all look fine to me.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
@@ -500,21 +500,21 @@ static phandle_t rtems_ofw_get_effective_phandle(<br>
)<br>
{<br>
phandle_t child;<br>
- phandle_t ref;<br>
+ phandle_t ref[1];<br>
<br></blockquote><div><br></div><div>I don't like this. I know this was suggested, but I think it is a little ridiculous. This is a false positive since we know that sizeof(*buf) == len. I don't know if we can make that an assertion. Otherwise, we can just mark this as a false positive in coverity. We know the array dereference in this case won't overwrite past the bounds of ref.</div><div><br></div><div>Instead of using hard-coded values of 4 in rtems_ofw_get_enc_prop() you might make it more explicitly using sizeof(pcell_t), since that is what you mean.</div></div></div></blockquote><div><span class="gmail_default" style="font-size:small">Done.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>I would also agree to change the strncpy as Christian identified before in rtems_ofw_get_prop().</div></div></div></blockquote><div><span class="gmail_default" style="font-size:small">Is the reason to avoid strncpy that it ignores the null byte if len(dst) <= len(src)?</span> </div><div><div style="font-size:small">If so can I do an explicit null byte append?</div><div style="font-size:small">Or is there any other reason?</div><br></div></div></div></blockquote><div>The reason is that it passes void* pointers. If these are strings, you should use char* type. Otherwise, memcpy is more suitable.</div><div><br></div><div>It also would be generally safer to overwrite with the NIL character to guarantee it is a null-terminated string, if that is expected.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
for (child = rtems_ofw_child(node); child != 0; child = 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></blockquote><div><br></div><div>I didn't notice before, but is this recursion bounded (yes, it is a tree, but it might be better to rewrite this function non-recursively).</div></div></div></blockquote><div><span class="gmail_default" style="font-size:small">Just curious why is it better? Is it because it might use a lot of stack frames?</span></div><div><span class="gmail_default" style="font-size:small">I can only think of using stack or queue to implement it non-recursively. Is there</span></div><div><span class="gmail_default" style="font-size:small">any other way?</span></div></div></div></blockquote><div><br></div><div>Recursion causes two potential problems: large stack usage and hard-to-analyze execution times. These are generally important for an RTOS to be wary of.</div><div><br></div><div>This looks like a depth-first search to find xref? But the tree traversal order doesn't matter. In fact, I would check if the FDT can be iterated directly. I don't know enough about the FDT structure to say whether that is simple to do. If you start at the root and repeatedly call fdt_next_node() do you traverse all the nodes?</div><div><br></div><div>You can implement non-recursive tree searches using nested loops when you have sibling, child, and parent pointers. Probably, you can find code examples of how to do this. The basic idea is pretty simple though, here is a DFS:</div><div>node = root</div><div>do {</div><div> visit(node)</div><div> next_node = child_of(node)</div><div> if ( ! next_node ) {</div><div> while ( !has_sibling(node) && node != root) {<br></div><div> node = parent_of(node) /* back up */</div><div> }</div><div> next_node = sibling_of(node)</div><div> }</div><div> node = next_node;</div><div>} while (node)</div><div><br></div><div>This pseudocode assumes the root has a NULL-value sibling and leaves have NULL-value children. I also didn't test it, but the rough idea should work. You can do something similar with BFS.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ if (ref[0] != -1)<br>
+ return ref[0];<br>
<br>
- if (rtems_ofw_get_enc_prop(child, "phandle", &ref, sizeof(ref)) == -1 &&<br>
- rtems_ofw_get_enc_prop(child, "ibm,phandle", &ref, sizeof(ref)) == -1 &&<br>
- rtems_ofw_get_enc_prop(child, "linux,phandle", &ref, sizeof(ref)) == -1<br>
+ if (rtems_ofw_get_enc_prop(child, "phandle", ref, sizeof(ref)) == -1 &&<br>
+ rtems_ofw_get_enc_prop(child, "ibm,phandle", ref, sizeof(ref)) == -1 &&<br>
+ rtems_ofw_get_enc_prop(child, "linux,phandle", ref, 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( 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, sizeof(ref)) == -1 &&<br>
- rtems_ofw_get_enc_prop(node, "ibm,phandle", &ref, sizeof(ref)) == -1 &&<br>
- rtems_ofw_get_enc_prop(node, "linux,phandle", &ref, sizeof(ref)) == -1)<br>
+ if (rtems_ofw_get_enc_prop(node, "phandle", ref, sizeof(ref)) == -1 &&<br>
+ rtems_ofw_get_enc_prop(node, "ibm,phandle", ref, sizeof(ref)) == -1 &&<br>
+ rtems_ofw_get_enc_prop(node, "linux,phandle", ref, 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><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>
</blockquote></div></div>
</blockquote></div></div>