[PATCH v3] bsps/shared/ofw: Fix coverity reported defects
Niteesh G. S.
niteesh.gs at gmail.com
Fri Feb 5 08:45:57 UTC 2021
On Fri, Feb 5, 2021 at 12:22 AM Christian Mauderer <contact at c-mauderer.de>
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.
OK.
Summarizing changes since the last posted patch (v3)
1) Using a variable instead of an array for CID 1472597, 1472595
so this has to be marked as a false positive in Coverity since we
guarantee that we don't overflow.
2) rtems_ofw_get_effective_phandle is now iterative instead of recursive.
3) Using memcpy instead of strncpy
4) I have also fixed another bug, should I post it as part of this patch or
a
separate patch?
Thanks,
Niteesh.
> 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210205/0fd4d7de/attachment-0001.html>
More information about the devel
mailing list