<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, 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 class="gmail_default" style="font-size:small">If so can I do an explicit null byte append?</div><div class="gmail_default" style="font-size:small">Or is there any other reason?</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"><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><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>