[U-Boot] Potential memory corruption in drivers/net/sh_eth.c ?

[Heiko Schocher]

Hello Nobuhiro,

Am 07.04.2016 um 22:31 schrieb Wolfgang Denk:
> Dear Nobuhiro,
>
> while tracking down a memory corruption bug in other code, I ran over
> these lines in  drivers/net/sh_eth.c :
>
> ...
> 194         /*
> 195          * Allocate rx descriptors. They must be aligned to size of struct
> 196          * tx_desc_s.
> 197          */
> 198         port_info->tx_desc_alloc =
> 199                 memalign(sizeof(struct tx_desc_s), alloc_desc_size);
>
> ...
> 246         /*
> 247          * Allocate rx descriptors. They must be aligned to size of struct
> 248          * rx_desc_s.
> 249          */
> 250         port_info->rx_desc_alloc =
> 251                 memalign(sizeof(struct rx_desc_s), alloc_desc_size);
>
>
> There is some padding done (in drivers/net/sh_eth.h) to the stucts
> tx_desc_s and rx_desc_s, but it appears onecritical fact is nowhere
> checked:
>
> Quoting from "common/dlmalloc.c":
>
> ....
> 2784   memalign algorithm:
> 2785
> 2786     memalign requests more than enough space from malloc, finds a spot
> 2787     within that chunk that meets the alignment request, and then
> 2788     possibly frees the leading and trailing space.
> 2789
> 2790     The alignment argument must be a power of two. This property is not
> 2791     checked by memalign, so misuse may result in random runtime errors.
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> I. e. it is _mandatory_ that the first argument to memalign() must be
> a power of two.  The current code does not guarantee this, and the
> comments in the code (drivers/net/sh_eth.h) do not hint on this
> restriction either:
>
> ...
>   51 /* The size of the tx descriptor is determined by how much padding is used.
>   52    4, 20, or 52 bytes of padding can be used */
>
> I recommend to make this restriction more visible in the code and in
> the comment, and/or even add a compile time test to guarantee this
> requirement is met.

Maybe you try the following patch on your hardware:

diff --git a/common/dlmalloc.c b/common/dlmalloc.c
index b5bb051..cb2a5d8 100644
--- a/common/dlmalloc.c
+++ b/common/dlmalloc.c
@@ -2797,6 +2797,15 @@ Void_t* rEALLOc(oldmem, bytes) Void_t* oldmem; size_t bytes;

  */

+/*
+ * from:
+ * http://www.exploringbinary.com/ten-ways-to-check-if-an-integer-is-a-power-of-two-in-c/
+ * "10. Complement and Compare"
+ */
+static int isPowerOfTwo(size_t x)
+{
+       return ((x != 0) && ((x & (~x + 1)) == x));
+}

  #if __STD_C
  Void_t* mEMALIGn(size_t alignment, size_t bytes)
@@ -2824,6 +2833,12 @@ Void_t* mEMALIGn(alignment, bytes) size_t alignment; size_t bytes;

    if (alignment <  MINSIZE) alignment = MINSIZE;

+  /* check if alignment is a power of 2 */
+  if (isPowerOfTwo(alignment) == 0) {
+    printf("%s: %d is not power of 2!\n", __func__, alignment);
+    return NULL;
+  }
+
    /* Call malloc with worst case padding to hit alignment. */

    nb = request2size(bytes);

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany