On 04/02/2021 17:34, Gedare Bloom wrote:
On Thu, Feb 4, 2021 at 1:58 AM Niteesh G. S. <niteesh...@gmail.com
<mailto:niteesh...@gmail.com>> wrote:
On Thu, Feb 4, 2021 at 1:21 AM Gedare Bloom <ged...@rtems.org
<mailto:ged...@rtems.org>> wrote:
On Wed, Feb 3, 2021 at 2:48 AM G S Niteesh Babu
<niteesh...@gmail.com <mailto:niteesh...@gmail.com>> wrote:
Fixed use after free and null pointer dereference defects
FIXES:
1) CID 1472601 (NULL_RETURNS)
2) CID 1472600 (USE_AFTER_FREE)
3) CID 1472599 (USE_AFTER_FREE)
4) CID 1472598 (USE_AFTER_FREE)
5) CID 1472596 (USE_AFTER_FREE)
6) CID 1472597 (ARRAY_VS_SINGLETON)
7) CID 1472595 (ARRAY_VS_SINGLETON)
---
bsps/shared/ofw/ofw.c | 36
++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/bsps/shared/ofw/ofw.c b/bsps/shared/ofw/ofw.c
index 82924b2600..ccd57e36af 100644
--- a/bsps/shared/ofw/ofw.c
+++ b/bsps/shared/ofw/ofw.c
@@ -313,7 +313,7 @@ ssize_t rtems_ofw_get_prop_alloc(
}
if (rtems_ofw_get_prop(node, propname, *buf, len) ==
-1) {
- rtems_ofw_free(buf);
+ rtems_ofw_free(*buf);
*buf = NULL;
return -1;
}
@@ -344,7 +344,7 @@ ssize_t rtems_ofw_get_prop_alloc_multi(
}
if (rtems_ofw_get_prop(node, propname, *buf, len) ==
-1) {
- rtems_ofw_free(buf);
+ rtems_ofw_free(*buf);
*buf = NULL;
return -1;
}
@@ -373,7 +373,7 @@ ssize_t rtems_ofw_get_enc_prop_alloc(
}
if (rtems_ofw_get_enc_prop(node, propname, *buf, len)
== -1) {
- rtems_ofw_free(buf);
+ rtems_ofw_free(*buf);
*buf = NULL;
return -1;
}
@@ -404,7 +404,7 @@ ssize_t
rtems_ofw_get_enc_prop_alloc_multi(
}
if (rtems_ofw_get_enc_prop(node, propname, *buf, len)
== -1) {
- rtems_ofw_free(buf);
+ rtems_ofw_free(*buf);
*buf = NULL;
return -1;
}
The above all look fine to me.
@@ -500,21 +500,21 @@ static phandle_t
rtems_ofw_get_effective_phandle(
)
{
phandle_t child;
- phandle_t ref;
+ phandle_t ref[1];
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.
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.
Done.
I would also agree to change the strncpy as Christian identified
before in rtems_ofw_get_prop().
Is the reason to avoid strncpy that it ignores the null byte if
len(dst) <= len(src)?
If so can I do an explicit null byte append?
Or is there any other reason?
The reason is that it passes void* pointers. If these are strings, you
should use char* type. Otherwise, memcpy is more suitable.
It also would be generally safer to overwrite with the NIL character
to guarantee it is a null-terminated string, if that is expected.
That was not the only reason. Let me pull the relevant lines together
(reordered and pulled from multiple files):
typedef uint32_t pcell_t;
rtems_ofw_get_enc_prop(
phandle_t node,
const char *prop,
pcell_t *buf,
size_t len
)
{
...
rv = rtems_ofw_get_prop(node, prop, buf, len);
...
}
rtems_ofw_get_prop(
phandle_t node,
const char *propname,
void *buf,
size_t bufsize
{
...
strncpy(buf, prop, bufsize);
...
}
Let's say I do the following call:
rtems_ofw_get_enc_prop(node, "name", &foo, sizeof(foo));
In that case the code is using a strncpy to copy into a uint32_t. That's
not a good idea. What if there is (for example) a value of 0x00110011 in
the property? strncpy will find one of these two 0 bytes and stop there.
I'm not sure which one because endianess will have an influence on that
too. Note that I'm not sure whether using rtems_ofw_get_enc_prob with
these parameters is a useful call. But it's possible and it's a bad idea
if it results in an undefined behavior.
Best regards
Christian
for (child = rtems_ofw_child(node); child != 0; child =
rtems_ofw_peer(child)) {
- ref = rtems_ofw_get_effective_phandle(child, xref);
- if (ref != -1)
- return ref;
+ ref[0] = rtems_ofw_get_effective_phandle(child, xref);
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).
Just curious why is it better? Is it because it might use a lot of
stack frames?
I can only think of using stack or queue to implement it
non-recursively. Is there
any other way?
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.
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?
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:
node = root
do {
visit(node)
next_node = child_of(node)
if ( ! next_node ) {
while ( !has_sibling(node) && node != root) {
node = parent_of(node) /* back up */
}
next_node = sibling_of(node)
}
node = next_node;
} while (node)
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.
+ if (ref[0] != -1)
+ return ref[0];
- if (rtems_ofw_get_enc_prop(child, "phandle", &ref,
sizeof(ref)) == -1 &&
- rtems_ofw_get_enc_prop(child, "ibm,phandle", &ref,
sizeof(ref)) == -1 &&
- rtems_ofw_get_enc_prop(child, "linux,phandle",
&ref, sizeof(ref)) == -1
+ if (rtems_ofw_get_enc_prop(child, "phandle", ref,
sizeof(ref)) == -1 &&
+ rtems_ofw_get_enc_prop(child, "ibm,phandle", ref,
sizeof(ref)) == -1 &&
+ rtems_ofw_get_enc_prop(child, "linux,phandle", ref,
sizeof(ref)) == -1
) {
continue;
}
- if (ref == xref)
+ if (ref[0] == xref)
return child;
}
@@ -533,16 +533,16 @@ phandle_t rtems_ofw_node_from_xref(
phandle_t xref )
phandle_t rtems_ofw_xref_from_node( phandle_t node )
{
- phandle_t ref;
+ phandle_t ref[1];
- if (rtems_ofw_get_enc_prop(node, "phandle", &ref,
sizeof(ref)) == -1 &&
- rtems_ofw_get_enc_prop(node, "ibm,phandle", &ref,
sizeof(ref)) == -1 &&
- rtems_ofw_get_enc_prop(node, "linux,phandle", &ref,
sizeof(ref)) == -1)
+ if (rtems_ofw_get_enc_prop(node, "phandle", ref,
sizeof(ref)) == -1 &&
+ rtems_ofw_get_enc_prop(node, "ibm,phandle", ref,
sizeof(ref)) == -1 &&
+ rtems_ofw_get_enc_prop(node, "linux,phandle", ref,
sizeof(ref)) == -1)
{
return node;
}
- return ref;
+ return ref[0];
}
phandle_t rtems_ofw_instance_to_package( ihandle_t
instance )
@@ -614,7 +614,7 @@ int rtems_ofw_get_reg(
offset = rtems_fdt_phandle_to_offset(parent);
ptr = fdt_getprop(fdtp, offset, "ranges", &len);
- if (len < 0) {
+ if (ptr == NULL) {
break;
}
-- 2.17.1
_______________________________________________
devel mailing list
devel@rtems.org <mailto:devel@rtems.org>
http://lists.rtems.org/mailman/listinfo/devel
<http://lists.rtems.org/mailman/listinfo/devel>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
