[U-Boot] Potential memory corruption in drivers/net/sh_eth.c ?
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.
Thanks.
Best regards,
Wolfgang Denk
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... + * "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
Dear Heiko,
[Nobuhiro's mail address fixed]
In message 57076C13.50203@denx.de you wrote:
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:
I think this is not needed. I reviewed the code, and if I'm not wrong the code adds exactly the needed padding to make the structure size a power of 2.
My complaint is that this requirement is not visible in the code, and there is no explanation for the magic numbers. The comment just says
The size of the tx descriptor is determined by how much padding is used. 4, 20, or 52 bytes of padding can be used
When you look closer, you will see that the currently used struct size is 12 bytes, so adding 4, 20, or 52 bytes of padding will result in total sizes of 12+4=16, 12+20=32 resp. 12+52=64 bytes, i. e. all will result in proper sizes.
But this is not obvious to the reader who does not happen to realize that this size is used as argument to memalign(), and that memalign() has such a mandatory requirement() without checking for it, and with potentially fatal consequences.
[And yes, I think we shoudl augment the memalign() implementation with a compile time test for proper alignment size... Any volunteers?]
Best regards,
Wolfgang Denk
Hi,
Sorry, reply was too late.
My complaint is that this requirement is not visible in the code, and there is no explanation for the magic numbers. The comment just says
The size of the tx descriptor is determined by how much padding is used. 4, 20, or 52 bytes of padding can be usedWhen you look closer, you will see that the currently used struct size is 12 bytes, so adding 4, 20, or 52 bytes of padding will result in total sizes of 12+4=16, 12+20=32 resp. 12+52=64 bytes, i. e. all will result in proper sizes.
Indeed. I will write more infomation about this padding.
Best regards, Nobuhiro
2016-04-08 17:55 GMT+09:00 Wolfgang Denk wd@denx.de:
Dear Heiko,
[Nobuhiro's mail address fixed]
In message 57076C13.50203@denx.de you wrote:
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:
I think this is not needed. I reviewed the code, and if I'm not wrong the code adds exactly the needed padding to make the structure size a power of 2.
My complaint is that this requirement is not visible in the code, and there is no explanation for the magic numbers. The comment just says
The size of the tx descriptor is determined by how much padding is used. 4, 20, or 52 bytes of padding can be usedWhen you look closer, you will see that the currently used struct size is 12 bytes, so adding 4, 20, or 52 bytes of padding will result in total sizes of 12+4=16, 12+20=32 resp. 12+52=64 bytes, i. e. all will result in proper sizes.
But this is not obvious to the reader who does not happen to realize that this size is used as argument to memalign(), and that memalign() has such a mandatory requirement() without checking for it, and with potentially fatal consequences.
[And yes, I think we shoudl augment the memalign() implementation with a compile time test for proper alignment size... Any volunteers?]
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de Die Jahre hinterlassen vielleicht Falten auf der Haut, aber die Begeisterung zu verlieren hinterlässt Falten auf der Seele. - Samuel Ullmann
participants (3)
-
Heiko Schocher -
Nobuhiro Iwamatsu -
Wolfgang Denk