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

[Nobuhiro Iwamatsu]

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 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.

Indeed. I will write more infomation about this padding.

Best regards,
  Nobuhiro

2016-04-08 17:55 GMT+09:00 Wolfgang Denk <wd at denx.de>:
> Dear Heiko,
>
> [Nobuhiro's mail address fixed]
>
> In message <57076C13.50203 at 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
>
> --
> 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 at denx.de
> Die Jahre hinterlassen vielleicht Falten auf der Haut, aber die
> Begeisterung zu verlieren hinterlässt Falten auf der Seele.
>                                                     - Samuel Ullmann



-- 
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6