Am 05.02.21 um 09:45 schrieb Niteesh G. S.:
On Fri, Feb 5, 2021 at 12:22 AM Christian Mauderer
<cont...@c-mauderer.de <mailto: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>
> <mailto: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>
> <mailto: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>
<mailto: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?
I would suggest to create a separate patch or a patch set. This one is
quite Coverity related. 2, 3 and 4 are improvements or bug fixes and
don't have to do anything with Coverity.
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> <mailto:devel@rtems.org
<mailto:devel@rtems.org>>
> http://lists.rtems.org/mailman/listinfo/devel
<http://lists.rtems.org/mailman/listinfo/devel>
> <http://lists.rtems.org/mailman/listinfo/devel
<http://lists.rtems.org/mailman/listinfo/devel>>
>
>
> _______________________________________________
> 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
--
--------------------------------------------
embedded brains GmbH
Herr Christian MAUDERER
Dornierstr. 4
82178 Puchheim
Germany
email: christian.maude...@embedded-brains.de
phone: +49-89-18 94 741 - 18
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
