[PATCH v3] bsps/shared/ofw: Fix coverity reported defects
Gedare Bloom
gedare at rtems.org
Thu Feb 4 16:34:17 UTC 2021
On Thu, Feb 4, 2021 at 1:58 AM Niteesh G. S. <niteesh.gs at gmail.com> wrote:
>
>
> On Thu, Feb 4, 2021 at 1:21 AM Gedare Bloom <gedare at rtems.org> wrote:
>
>>
>>
>> On Wed, Feb 3, 2021 at 2:48 AM G S Niteesh Babu <niteesh.gs at 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.
>
>>
>>> 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 at rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210204/db0ac85e/attachment.html>
More information about the devel
mailing list