[PATCH v3] bsps/shared/ofw: Fix coverity reported defects

[Gedare Bloom]

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.

I would also agree to change the strncpy as Christian identified before in
rtems_ofw_get_prop().


>    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).


> +    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/20210203/99e70069/attachment.html>