On Fri, Feb 5, 2021 at 12:22 AM Christian Mauderer <cont...@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...@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. 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@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 > > >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
