[PATCH v3] bsps/shared/ofw: Fix coverity reported defects
Christian Mauderer
oss at c-mauderer.de
Thu Feb 4 18:52:32 UTC 2021
On 04/02/2021 19:52, Christian Mauderer wrote:
>
>
> On 04/02/2021 17:34, Gedare Bloom wrote:
>>
>>
>> On Thu, Feb 4, 2021 at 1:58 AM Niteesh G. S. <niteesh.gs at gmail.com
>> <mailto:niteesh.gs at gmail.com>> wrote:
>>
>>
>>
>> On Thu, Feb 4, 2021 at 1:21 AM Gedare Bloom <gedare at rtems.org
>> <mailto:gedare at rtems.org>> wrote:
>>
>>
>>
>> On Wed, Feb 3, 2021 at 2:48 AM G S Niteesh Babu
>> <niteesh.gs at gmail.com <mailto: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.
>
> 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 at rtems.org <mailto:devel at rtems.org>
>> http://lists.rtems.org/mailman/listinfo/devel
>> <http://lists.rtems.org/mailman/listinfo/devel>
>>
>>
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
>>
More information about the devel
mailing list