Wolfgang Denk wrote:
I reject your patch because it introduces the risk of crashing the system and it appears you do not want to be bothered fixing this.
But I believe I have already fixed it by stating that any users of fdt_increase_size() must ensure that the new size fits in the allocated area. The same rules apply to functions like strcat(), so I don't understand why you are so concerned.
And I said that this is not sufficient. Static buffers have always been a bad idea as there will always be a user who needs more space. In this case, where we know the amount needed, we can as well arrange for it. You may have to add some code to implemenmt this, butthen adding this code has to be part of your patch submission.
But there is no way to ensure that every user of fdt_increase_size() will follow those rules.
I could, for instance, add an lmb parameter to fdt_increase_size(), but this will only apply in instances where the fdt exists inside an lmb. There is no lmb_realloc() function, so the most I can do is return an error if the lmb isn't big enough.
The problem is that by the time the code needs to resize the fdt, it has long since forgotten about the lmb. We would need to modify ft_board_setup() to take an lmb as a parameter, but that would require changes to almost 30 boards!
And then, that doesn't help the situation where the fdt is just dumped in memory somewhere, like this:
tftp 0xc0000 my.dtb bootm xxx xxx 0xc0000
In this situation, no lmb is created, so there's no way to know if there's anything at address 0xc4000 that could be overwritten.
So the best I can do is this:
int fdt_increase_size(struct lmb *lmb, void *fdt, int add_len) { int newlen;
newlen = fdt_totalsize(fdt) + add_len;
if (lmb) { int lmb_size = lmb_size(lmb);
if (newlen > lmb_size) return -ENOMEM; }
/* Open in place with a new len */ return fdt_open_into(fdt, fdt, newlen); }
So if an lmb is provided, we can check for overwrites, but we can't require the lmb.
The only problem with the above function is that lmb_size() doesn't exist, and I'm not sure how to implement it yet. There doesn't appear to be any useful support functions for the lmb code at the moment.
Right. We don't provide safety belts against doing stupid things. But you mentioned your extension is needed to insert firmware blobs which can take "significant size" - you cannot estimate in advace (at compile time) which size the end user may need.
Actually, I can. For firmware, at least, I have macro called CONFIG_SYS_FSL_FW_MAXSIZE that defines the largest size that firmware can be. This is used not only to specify the new size of the lmb (if applicable), but to also ensure limits on the firmware itself. This would be a board-specific setting, since each board knows what kind of firmware it needs. And then I have this:
#ifndef CONFIG_SYS_FDT_PAD_DFLT #define CONFIG_SYS_FDT_PAD_DFLT 0x3000 #endif
#ifndef CONFIG_SYS_FMAN_FW_MAXSIZE #define CONFIG_SYS_FMAN_FW_MAXSIZE 0 #endif
#ifndef CONFIG_SYS_FDT_PAD #define CONFIG_SYS_FDT_PAD (CONFIG_SYS_FDT_PAD_DFLT + CONFIG_SYS_FMAN_FW_MAXSIZE) #endif
Thus, each board can define its own settings, and boot_relocate_fdt() will honor them.
So using a static buffer size is a stupid thing too do as is is bound to fail rather sooner than later.
AFAIK, lmb regions cannot be resized, so once we create an lmb region for the fdt, we're stuck with it. That's why I've enhanced boot_relocate_fdt() to ensure there's enough room in the lmb region to hold the firmware. I just haven't posted *that* code yet, since it's part of the P4080DS board support that isn't ready yet.
I will not accept code that relies on such compile time assumptions about dynamically inserted data.
I don't see any way around that.
The firmware is inserted after the lmb is created, and the code that creates the lmb region has no knowledge of the firmware (or any other board-specific entities that are going to be inserted into the fdt). The best I can do is force the firmware to be smaller than a compile-time limit, and ensure the lmb region is expanded by that value.
[Note that it does not help that *you* *shout* *at* *me* all the time that you need this *now*. This does not make your patch any better.]
Shouting is done with ALL CAPS. Framing words with asterisks is used to designate emphasis.