[U-Boot-Users] [PATCH v3] Fix initrd length miscalculation in bootm command when using a dtu
The do_bootm_linux() function uses the same variable ('len') to calculate the dtu (flat device tree wrapped in a uImage) length and the initrd length, which means that the initrd length was incorrect when it was needed. This patch changes any code that uses 'len' or 'data' as temporary variables to use the values directly instead. It also adds a comment to the definitions of 'len' and 'data' reminding other programmers what they represent.
Signed-off-by: Timur Tabi timur@freescale.com ---
This patch is a simplied version of the initrd patch that only fixes the root problem, without renaming any variables. This bug was introduced in commit 87a449c8ac396420cb24260f717ea9e6faa82047.
common/cmd_bootm.c | 12 ++++-------- 1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 32c29e5..bd6d1c9 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -523,11 +523,11 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag, int verify) { ulong sp; - ulong len, checksum; + ulong len, data; /* The initrd length and address */ ulong initrd_start, initrd_end; ulong cmd_start, cmd_end; ulong initrd_high; - ulong data; + ulong checksum; int initrd_copy_to_ram = 1; char *cmdline; char *s; @@ -652,13 +652,10 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag, do_reset (cmdtp, flag, argc, argv); }
- data = (ulong)&header; - len = sizeof(image_header_t); - checksum = ntohl(hdr->ih_hcrc); hdr->ih_hcrc = 0;
- if (crc32 (0, (uchar *)data, len) != checksum) { + if (crc32 (0, (uchar *) &header, sizeof(image_header_t)) != checksum) { puts ("Bad Header Checksum\n"); SHOW_BOOT_PROGRESS (-11); do_reset (cmdtp, flag, argc, argv); @@ -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))) { printf("ERROR: Flat Device Tree checksum is invalid\n"); return; } -- 1.5.0.2.260.g2eb065
Dear Timur,
in message 11782314361072-git-send-email-timur@freescale.com you wrote:
The do_bootm_linux() function uses the same variable ('len') to calculate the dtu (flat device tree wrapped in a uImage) length and the initrd length, which means that the initrd length was incorrect when it was needed. This patch changes any code that uses 'len' or 'data' as temporary variables to use the values directly instead. It also adds a comment to the definitions of 'len' and 'data' reminding other programmers what they represent.
...
This patch is a simplied version of the initrd patch that only fixes the root problem, without renaming any variables. This bug was introduced in commit 87a449c8ac396420cb24260f717ea9e6faa82047.
...
@@ -523,11 +523,11 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag, int verify) { ulong sp;
- ulong len, checksum;
- ulong len, data; /* The initrd length and address */ ulong initrd_start, initrd_end; ulong cmd_start, cmd_end; ulong initrd_high;
- ulong data;
- ulong checksum; int initrd_copy_to_ram = 1; char *cmdline; char *s;
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.
@@ -652,13 +652,10 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag, do_reset (cmdtp, flag, argc, argv); }
data = (ulong)&header;len = sizeof(image_header_t);checksum = ntohl(hdr->ih_hcrc); hdr->ih_hcrc = 0;
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.
@@ -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.
With this patch it is impossible to see what the problem is you are trying to fix.
Looking at the patch, I see only cosmetical changes, which actually lead to less readable code.
If there is a problem, please fix the problem, but don't touch innocent code that has been there for ages.
Best regards,
Wolfgang Denk
Wolfgang Denk 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.
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.
@@ -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.
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.
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=9877d7dcd1eebe...
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
Wolfgang Denk wrote:
I'm glad you fixed the bug, so I'll just add a few comments:
Your comment is actually wrong. The use of "len" is not limited to that purpose.
If you apply my patch, then the comment becomes correct. My goal was to lock the variables 'len' and 'data' into one purpose. The reason the bug existed is because the other developer didn't realize this. He used 'len' thinking it was available. In a sense, I was trying to implement some "defensive programming", so that the next time someone hacks up do_bootm_linux(), he won't re-introduce the bug.
Now, you might say that that won't happen again, but I disagree. I think it can, for two reasons:
1) It happened once already, last year. You approved and applied a patch which does overwrite the variable.
2) The libfdt code introduced a number of other bugs relating to dtu usage, which have not yet been fixed.
So I believe there is a real possibility that another patch could re-introduce this bug. If you had applied my patch as-is, that possibility would have been eliminated. This is why I think my patch is better than yours.
But I guess only time will tell who's right. :-)
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.]
Stefan said he had a testing repo of some kind. How about we just use that? If Stefan is willing, he can apply my emailed patches to his repo.
Hi Timur,
On Friday 04 May 2007 15:25, Timur Tabi wrote:
I'm glad you fixed the bug, so I'll just add a few comments:
Your comment is actually wrong. The use of "len" is not limited to that purpose.
If you apply my patch, then the comment becomes correct. My goal was to lock the variables 'len' and 'data' into one purpose. The reason the bug existed is because the other developer didn't realize this. He used 'len' thinking it was available. In a sense, I was trying to implement some "defensive programming", so that the next time someone hacks up do_bootm_linux(), he won't re-introduce the bug.
Now, you might say that that won't happen again, but I disagree. I think it can, for two reasons:
- It happened once already, last year. You approved and applied a
patch which does overwrite the variable.
- The libfdt code introduced a number of other bugs relating to dtu
usage, which have not yet been fixed.
So I believe there is a real possibility that another patch could re-introduce this bug. If you had applied my patch as-is, that possibility would have been eliminated. This is why I think my patch is better than yours.
But I guess only time will tell who's right. :-)
You have a point with your variable usage "restriction", and Wolfgang has a point with the "readability" of your patch as it doesn't really tell what is fixed without read the current source. Could be that your implementation is the "better" one, but the patch Wolfgang applied was just less intrusive.
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.]
Stefan said he had a testing repo of some kind. How about we just use that? If Stefan is willing, he can apply my emailed patches to his repo.
No, we shouldn't "fork" the code here. Let's focus on the other open issues. You mention other bugs introduced by the libfdt code above. ;-)
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk Office: Kirchenstr. 5, D-82194 Groebenzell, Germany =====================================================================
Stefan Roese wrote:
Hi Timur,
On Friday 04 May 2007 15:25, Timur Tabi wrote:
I'm glad you fixed the bug, so I'll just add a few comments:
Your comment is actually wrong. The use of "len" is not limited to that purpose.
If you apply my patch, then the comment becomes correct. My goal was to lock the variables 'len' and 'data' into one purpose. The reason the bug existed is because the other developer didn't realize this. He used 'len' thinking it was available. In a sense, I was trying to implement some "defensive programming", so that the next time someone hacks up do_bootm_linux(), he won't re-introduce the bug.
Now, you might say that that won't happen again, but I disagree. I think it can, for two reasons:
- It happened once already, last year. You approved and applied a
patch which does overwrite the variable.
- The libfdt code introduced a number of other bugs relating to dtu
usage, which have not yet been fixed.
So I believe there is a real possibility that another patch could re-introduce this bug. If you had applied my patch as-is, that possibility would have been eliminated. This is why I think my patch is better than yours.
But I guess only time will tell who's right. :-)
You have a point with your variable usage "restriction", and Wolfgang has a point with the "readability" of your patch as it doesn't really tell what is fixed without read the current source. Could be that your implementation is the "better" one, but the patch Wolfgang applied was just less intrusive.
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.]
Stefan said he had a testing repo of some kind. How about we just use that? If Stefan is willing, he can apply my emailed patches to his repo.
No, we shouldn't "fork" the code here. Let's focus on the other open issues. You mention other bugs introduced by the libfdt code above. ;-)
Best regards, Stefan
Timur has sent me two libfdt/dtu-related fixes * fdt_copy_into() had the source and destination addresses reversed * fdt_check_header() had the wrong sense on ==/!= 0 which I applied to my local repo last night and will push back to u-boot-fdt soon (I still have not been successful in figuring out how to make a multi-image to test the changes grrrr). The two bugs I introduced in the conversion to libfdt were unrelated to the "len" variable, but I probably replicated the "len" bug when I duplicated and modified the original code. I will pull Wolfgang's fix tonight and see what needs to be done to make all three bugs go away.
WRT to building a multi-image, I'm looking to combine linux, a dtb, and an initrd into a single image to test that path of bootm (the path I had the above screwups in).
Timur's hint for me was:
mkimage -A ppc -T flat_dt -C none -d mpc836x_mds.dtb mpc836x_mds.dtu
This, of course, creates a dtu with a value of 0 for ih_load. You probably need to specify -a or -e to set that field.
but I don't understand how to build an image with all three pieces in it (I tried to use the ":" in the -d source parameter and mkimage just got confused, couldn't find the files). Am I expecting too much??? Should I just be wrapping the three pieces individually and loading them separately? What exactly are you doing to test this, Timur?
Best regards, gvb
Jerry Van Baren wrote:
but I don't understand how to build an image with all three pieces in it (I tried to use the ":" in the -d source parameter and mkimage just got confused, couldn't find the files). Am I expecting too much??? Should I just be wrapping the three pieces individually and loading them separately? What exactly are you doing to test this, Timur?
The 'len' bug only shows up if both of these conditions are met:
1) You're booting an OF-enabled kernel (i.e. there's an fdt) 2) The fdt is wrapped in a dtu image (type IH_TYPE_FLATDT)
I didn't test having the fdt merged in with other entities in a combined image. I just made a dtu and told the bootm command to use it. So if you want to test this code, I think just wrapping the three pieces individually should be sufficient.
Part of the problem is that the code looks for a dtu image. If you combine all three chunks into one image, then I don't know what the code will do, because the image type won't be IH_TYPE_FLATDT.
Timur Tabi wrote:
Jerry Van Baren wrote:
but I don't understand how to build an image with all three pieces in it (I tried to use the ":" in the -d source parameter and mkimage just got confused, couldn't find the files). Am I expecting too much??? Should I just be wrapping the three pieces individually and loading them separately? What exactly are you doing to test this, Timur?
The 'len' bug only shows up if both of these conditions are met:
- You're booting an OF-enabled kernel (i.e. there's an fdt)
- The fdt is wrapped in a dtu image (type IH_TYPE_FLATDT)
I didn't test having the fdt merged in with other entities in a combined image. I just made a dtu and told the bootm command to use it. So if you want to test this code, I think just wrapping the three pieces individually should be sufficient.
Part of the problem is that the code looks for a dtu image. If you combine all three chunks into one image, then I don't know what the code will do, because the image type won't be IH_TYPE_FLATDT.
Ahh, thank you. That clarifies what I was misunderstanding.
gvb
Stefan Roese wrote:
You have a point with your variable usage "restriction", and Wolfgang has a point with the "readability" of your patch as it doesn't really tell what is fixed without read the current source. Could be that your implementation is the "better" one, but the patch Wolfgang applied was just less intrusive.
Well, I can't argue with that.
No, we shouldn't "fork" the code here. Let's focus on the other open issues.
Since you bring it up, I submitted a patch for the 5xxx platform many months ago. How do I get that applied? The patch is here:
http://sourceforge.net/mailarchive/message.php?msg_id=117131648887-git-send-...
You mention other bugs introduced by the libfdt code above. ;-)
Jerry is already aware of them and working on fixes.
In message 463B3463.1030702@freescale.com you wrote:
If you apply my patch, then the comment becomes correct. My goal was to lock the variables 'len' and 'data' into one purpose. The reason the
I think you actually missed a few places where len was also used.
bug existed is because the other developer didn't realize this. He used 'len' thinking it was available. In a sense, I was trying to implement
This type of thinking was plain wrong. You must never use any variable without understanding what it's being used for.
Stefan said he had a testing repo of some kind. How about we just use that? If Stefan is willing, he can apply my emailed patches to his repo.
The u-boot-testing repo is actually *my* guardian repo where I put all stuff in that does not fit anywhere else.
As I explained before, you need a normal developer's repo, not a guardian one. And I repeat: I offer you such a repo, it's available right now, I just need the IP address from where you will connect.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message 463B3463.1030702@freescale.com you wrote:
If you apply my patch, then the comment becomes correct. My goal was to lock the variables 'len' and 'data' into one purpose. The reason the
I think you actually missed a few places where len was also used.
Really? I doubt it. Every other place that len is used, it's the initrd length. Can you show me where that's not the case?
bug existed is because the other developer didn't realize this. He used 'len' thinking it was available. In a sense, I was trying to implement
This type of thinking was plain wrong. You must never use any variable without understanding what it's being used for.
You're absolutely right, which is why *I* don't make that mistake. But some one else did, and someone else might again. do_bootm_linux() is hideous code, and it just gets worse and worse. Frankly, anyone trying to modify that code needs all the help he can get!
As I explained before, you need a normal developer's repo, not a guardian one. And I repeat: I offer you such a repo, it's available right now, I just need the IP address from where you will connect.
It would be the same IP address that Kim, Andy, and Jon use. As soon as I get hold of one of them, I'll ask, but technically you should know the answer already.
In message 463B6022.7000908@freescale.com you wrote:
Really? I doubt it. Every other place that len is used, it's the initrd length. Can you show me where that's not the case?
See for example line 656: len = sizeof(image_header_t);
It would be the same IP address that Kim, Andy, and Jon use. As soon as I get hold of one
And that is???
of them, I'll ask, but technically you should know the answer already.
No, I don't. That's why I ask.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
In message 463B6022.7000908@freescale.com you wrote:
Really? I doubt it. Every other place that len is used, it's the initrd length. Can you show me where that's not the case?
See for example line 656: len = sizeof(image_header_t);
Wolfgang, look at my patch!!!!
@@ -652,13 +652,10 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag, do_reset (cmdtp, flag, argc, argv); }
- data = (ulong)&header; - len = sizeof(image_header_t); - checksum = ntohl(hdr->ih_hcrc); hdr->ih_hcrc = 0;
- if (crc32 (0, (uchar *)data, len) != checksum) { + if (crc32 (0, (uchar *) &header, sizeof(image_header_t)) != checksum) { puts ("Bad Header Checksum\n"); SHOW_BOOT_PROGRESS (-11); do_reset (cmdtp, flag, argc, argv);
My patch deletes that usage of 'len'!!! *Your* patch doesn't!
It would be the same IP address that Kim, Andy, and Jon use. As soon as I get hold of one
And that is???
I just spoke to Andy, and he never gave you the IP address comes from, so I don't understand why you need from him but didn't need it from him. Regardless, I'm going to try to figure out the IP address, and I'll send you another email.
Wolfgang Denk wrote:
It would be the same IP address that Kim, Andy, and Jon use. As soon as I get hold of one
And that is???
I *think* the IP address is 192.88.165.35, aka gate-de.freescale.com. That's what external sites say my IP address is when I connect to them.
participants (4)
-
Jerry Van Baren -
Stefan Roese -
Timur Tabi -
Wolfgang Denk