[U-Boot-Users] [PATCH v3] Fix initrd length miscalculation in bootm command when using a dtu

Wolfgang Denk
2007-05-04T08:28:36Z

Dear Timur,

in message <463A88BA.2030607@freescale.com> you wrote:
>
> > So before your patch we had "len", "checksum", and "data", and now we
> > have "len", "data", and "checksum".
> >
> > What is your reason for this change? I see no functinal difference
> > nor any improvement of readability.
>
> It's for the comment that I added. I wanted to make sure that the reader understood what
> 'len' and 'data' are for, since their purpose is not obvious from just reading the code.

Your comment is actually wrong. The use of "len" is not limited to
that purpose.

But that doesn't mean that you can overwrite this variable without
checking.

> >> - if (crc32 (0, (uchar *)data, len) != checksum) {
> >> + if (crc32 (0, (uchar *) &header, sizeof(image_header_t)) != checksum) {
> >
> > This looks functionally identical to me, but is less readable.
>
> The point behind the patch is to make sure that 'len' and 'data' are only used for the
> purpose they are intended. Here, len and data are used as temporary variables.

...and here this is perfectly legal, as "len" does not store any
other value that is needed later.

> >> @@ -779,9 +776,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
> >>
> >> checksum = ntohl(hdr->ih_dcrc);
> >> addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
> >> - len = ntohl(hdr->ih_size);
> >>
> >> - if(checksum != crc32(0, (uchar *)addr, len)) {
> >> + if(checksum != crc32(0, (uchar *)addr, ntohl(hdr->ih_size))) {
> >
> > Same here. functionally identical but less readable.
>
> Not true. In this case, len has already been assigned the length of the initrd, and here
> we lose this value. Without this change, the kernel will panic at boot.

Right. I agree that this is the core of the problem, but from your
patch it was impossible to see. You have to read the diff, plus your
patch description, plus the code.

I think we should just fix the bug, which is the incorrect use of the
variable "len" at a place where it was already used. As Johns Daniel
pointed out (see his message from Tue, 27 Mar 2007 11:25:55 -0500),
this is the core of your poatch and all that needs to be changed.

> > With this patch it is impossible to see what the problem is you are
> > trying to fix.
>
> It's clearly explained in the comment: 'data' and 'len' are the address and length of the
> initrd, so this patch makes sure that these variables only contain those values.

No, this explanation is wrong. "len" gets used for some other
purposes, too.

To put this problem finally to rest I checked in the fix as suggested
by Johns Daniel, i. e. the core idea of your patch. See
http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=commit;h=9877d7dcd1eebe61aa5d8b8ffe9c048ea426e6f6

Thanks for pointing out the problem and providing a fix.

And please accept my apologies thatt his was so complicated and took
so long. [Nevertheless you still might want to try to find a way to
access the repository I created for you in case you have more
patches.]

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, CEO: Wolfgang Denk & Detlev Zundel
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
"My name is Linus Torvalds, you messed with my kernel, prepare to die"
- Linus Torvalds in
<Pine.LNX.3.91.960426110644.24860I-100000@linux.cs.Helsinki.FI>