[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely
Hi,
I noticed that the monitor flash autoprotection from drivers/cfi_flash.c is not completely safe. It does not protect all bootloader sectors in some situations:
/* Monitor protection ON by default */ #if (CFG_MONITOR_BASE >= CFG_FLASH_BASE) flash_protect (FLAG_PROTECT_SET, CFG_MONITOR_BASE, CFG_MONITOR_BASE + monitor_flash_len - 1, flash_get_info(CFG_MONITOR_BASE)); #endif
I noticed this on the APC405 board after modifying it to use the common CFI flash driver. Here are some lines from its config file:
#define CFG_MONITOR_BASE 0xFFF80000 #define CFG_MONITOR_LEN (512 * 1024) /* Reserve 512 kB for Monitor */
#define CFG_FLASH_CFI 1 /* Flash is CFI conformant */ #define CFG_FLASH_CFI_DRIVER 1 /* Use the common driver */ #define CFG_MAX_FLASH_SECT 128 /* max num of sects on one chip */ #define CFG_MAX_FLASH_BANKS 2 /* max num of flash banks */ #define CFG_FLASH_PROTECTION 1 /* use hardware protection */ #define CFG_FLASH_USE_BUFFER_WRITE 1 /* use buffered writes (20x faster) */ #define CFG_FLASH_BANKS_LIST {CFG_FLASH_BASE, CFG_FLASH_BASE + CFG_FLASH_INCREMENT}
With this setup 'CFG_MONITOR_BASE + monitor_flash_len - 1' evalutaes to fffc00ff and therefore the last sector will not be protected. But on 4xx CPUs you have the reset vector in the last sector so you definetely want it to be protected.
=> flinfo ... FFDC0000 FFDE0000 FFE00000 FFE20000 FFE40000 FFE60000 FFE80000 FFEA0000 FFEC0000 FFEE0000 E FFF00000 E FFF20000 E FFF40000 E FFF60000 E FFF80000 RO FFFA0000 RO FFFC0000 RO FFFE0000
Question: what's the best way to fix this? We could modify the call to flash_protect() like this:
flash_protect (FLAG_PROTECT_SET, CFG_MONITOR_BASE, CFG_MONITOR_BASE + CFG_MONITOR_LEN - 1, flash_get_info(CFG_MONITOR_BASE));
But I am not sure if this is fine for all architectures. Any ideas?
Matthias
In message 200711181959.55060.matthias.fuchs@esd-electronics.com you wrote:
I noticed that the monitor flash autoprotection from drivers/cfi_flash.c is not completely safe. It does not protect all bootloader sectors in some situations:
...
Question: what's the best way to fix this? We could modify the call to flash_protect()
The bst way is to make sure that all sectors that need to be protected do get protected.
like this:
flash_protect (FLAG_PROTECT_SET, CFG_MONITOR_BASE, CFG_MONITOR_BASE + CFG_MONITOR_LEN - 1, flash_get_info(CFG_MONITOR_BASE));
But I am not sure if this is fine for all architectures. Any ideas?
The problem is that you don't have any guarantee that CFG_MONITOR_LEN includes the reset vector; also, there might be configurations where the U-Boot image is not stored at the end of the flash, so there is a bigger gap between the image and the sector with the reset vector, and it would be not good to enforce protection on that area that might be useful to the user otherwise.
I think as a short term fix we might define an additional area that needs to be protected (the reset vector). Mid/long term we should change the code so you can pass a list of areas (start/end or start/length pairs) that will be protected. This would, for example, also allow to keep certain other areas (FDT, kernel image, etc.) auto-protected as well - configurable by the user ona per-board base.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Sunday 18 November 2007 20:31, Wolfgang Denk wrote:
In message 200711181959.55060.matthias.fuchs@esd-electronics.com you wrote:
I noticed that the monitor flash autoprotection from drivers/cfi_flash.c is not completely safe. It does not protect all bootloader sectors in some situations:
...
Question: what's the best way to fix this? We could modify the call to flash_protect()
The bst way is to make sure that all sectors that need to be protected do get protected.
sure:-)
like this:
flash_protect (FLAG_PROTECT_SET, CFG_MONITOR_BASE, CFG_MONITOR_BASE + CFG_MONITOR_LEN - 1, flash_get_info(CFG_MONITOR_BASE));
But I am not sure if this is fine for all architectures. Any ideas?
The problem is that you don't have any guarantee that CFG_MONITOR_LEN includes the reset vector; also, there might be configurations where the U-Boot image is not stored at the end of the flash, so there is a bigger gap between the image and the sector with the reset vector, and it would be not good to enforce protection on that area that might be useful to the user otherwise.
I think as a short term fix we might define an additional area that needs to be protected (the reset vector). Mid/long term we should change the code so you can pass a list of areas (start/end or start/length pairs) that will be protected. This would, for example, also allow to keep certain other areas (FDT, kernel image, etc.) auto-protected as well - configurable by the user ona per-board base.
Sometinhg like this?
Add this to board's config file: #define CONFIG_FLASH_AUTOPROTECT_LIST {{0xfffe0000, 0xffffffff}, {0xfe000000, 0xfe0fffff}}
And handle this list from the cfi driver:
diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c index 5579a1e..0777349 100644 --- a/drivers/cfi_flash.c +++ b/drivers/cfi_flash.c @@ -333,11 +333,17 @@ ulong flash_read_long (flash_info_t * info, flash_sect_t sect, uint offset)
/*----------------------------------------------------------------------- */ +struct apl_s {ulong start; ulong end;}; + unsigned long flash_init (void) { unsigned long size = 0; int i;
+#if defined(CONFIG_FLASH_AUTOPROTECT_LIST) + struct apl_s apl[] = CONFIG_FLASH_AUTOPROTECT_LIST; +#endif + #ifdef CFG_FLASH_PROTECTION char *s = getenv("unlock"); #endif @@ -419,6 +425,16 @@ unsigned long flash_init (void) CFG_ENV_ADDR_REDUND + CFG_ENV_SIZE_REDUND - 1, flash_get_info(CFG_ENV_ADDR_REDUND)); #endif + +#if defined(CONFIG_FLASH_AUTOPROTECT_LIST) + for (i = 0; i < sizeof(apl)/sizeof(struct apl_s); i++) { + printf("autoprotecting from %08x to %08x\n", apl[i].start, apl[i].end); + flash_protect (FLAG_PROTECT_SET, + apl[i].start, + apl[i].end, + flash_get_info(apl[i].start)); + } +#endif return (size); }
Good night.
Matthias
Dear Matthias,
in message 200711182214.06137.matthias.fuchs@esd-electronics.com you wrote:
I think as a short term fix we might define an additional area that needs to be protected (the reset vector). Mid/long term we should change the code so you can pass a list of areas (start/end or start/length pairs) that will be protected. This would, for example, also allow to keep certain other areas (FDT, kernel image, etc.) auto-protected as well - configurable by the user ona per-board base.
Sometinhg like this?
Add this to board's config file: #define CONFIG_FLASH_AUTOPROTECT_LIST {{0xfffe0000, 0xffffffff}, {0xfe000000, 0xfe0fffff}}
Yes, that's what I meant, although I have to admit that my personal preference would be to use { start, size } instead.
What do you (and others) think?
And handle this list from the cfi driver:
diff --git a/drivers/cfi_flash.c b/drivers/cfi_flash.c index 5579a1e..0777349 100644 --- a/drivers/cfi_flash.c +++ b/drivers/cfi_flash.c @@ -333,11 +333,17 @@ ulong flash_read_long (flash_info_t * info, flash_sect_t sect, uint offset)
/*----------------------------------------------------------------------- */ +struct apl_s {ulong start; ulong end;};
unsigned long flash_init (void) { unsigned long size = 0; int i;
+#if defined(CONFIG_FLASH_AUTOPROTECT_LIST)
struct apl_s apl[] = CONFIG_FLASH_AUTOPROTECT_LIST;+#endif
#ifdef CFG_FLASH_PROTECTION char *s = getenv("unlock"); #endif @@ -419,6 +425,16 @@ unsigned long flash_init (void) CFG_ENV_ADDR_REDUND + CFG_ENV_SIZE_REDUND - 1, flash_get_info(CFG_ENV_ADDR_REDUND)); #endif
+#if defined(CONFIG_FLASH_AUTOPROTECT_LIST)
for (i = 0; i < sizeof(apl)/sizeof(struct apl_s); i++) {printf("autoprotecting from %08x to %08x\n", apl[i].start, apl[i].end);flash_protect (FLAG_PROTECT_SET,apl[i].start,apl[i].end,flash_get_info(apl[i].start));}+#endif return (size); }
Right. That's what I had in mind. Except that I recommend to s/printf/debug/ ...
Best regards,
Wolfgang Denk
On Sunday 18 November 2007, Wolfgang Denk wrote:
I think as a short term fix we might define an additional area that needs to be protected (the reset vector). Mid/long term we should change the code so you can pass a list of areas (start/end or start/length pairs) that will be protected. This would, for example, also allow to keep certain other areas (FDT, kernel image, etc.) auto-protected as well - configurable by the user ona per-board base.
Sometinhg like this?
Add this to board's config file: #define CONFIG_FLASH_AUTOPROTECT_LIST {{0xfffe0000, 0xffffffff}, {0xfe000000, 0xfe0fffff}}
Yes, that's what I meant, although I have to admit that my personal preference would be to use { start, size } instead.
What do you (and others) think?
Yes, I would prefer size too. This would lead to something like this on 4xx boards:
#define CONFIG_FLASH_AUTOPROTECT_LIST { {CFG_MONITOR_BASE, CFG_MONITOR_LEN} }
Thanks.
Viele Grüße, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Hi,
On Monday 19 November 2007 07:14, Stefan Roese wrote:
On Sunday 18 November 2007, Wolfgang Denk wrote:
I think as a short term fix we might define an additional area that needs to be protected (the reset vector). Mid/long term we should change the code so you can pass a list of areas (start/end or start/length pairs) that will be protected. This would, for example, also allow to keep certain other areas (FDT, kernel image, etc.) auto-protected as well - configurable by the user ona per-board base.
Sometinhg like this?
Add this to board's config file: #define CONFIG_FLASH_AUTOPROTECT_LIST {{0xfffe0000, 0xffffffff}, {0xfe000000, 0xfe0fffff}}
Yes, that's what I meant, although I have to admit that my personal preference would be to use { start, size } instead.
What do you (and others) think?
I am passionsless. So I vote with the majority.
Yes, I would prefer size too. This would lead to something like this on 4xx boards:
#define CONFIG_FLASH_AUTOPROTECT_LIST { {CFG_MONITOR_BASE, CFG_MONITOR_LEN} }
This looks fine and I will start voting for this, too.
I am wondering if it is possible to get rid of the current autoprotection code (3 x flash_protect for monitor, env and 2nd env) in drivers/cfi_flash.c and fully implement it through the AUTOPROTECT_LIST without modifyiny all board config files. Any nice idea?
Especially on 4xx when you like to have sometinhg like
#define CONFIG_FLASH_AUTOPROTECT_LIST { {CFG_MONITOR_BASE, CFG_MONITOR_LEN} }
in your config file you will flash_protect most of the bootloader flash space twice.
Matthias
Hi Matthias,
On Monday 19 November 2007, Matthias Fuchs wrote:
Yes, I would prefer size too. This would lead to something like this on 4xx boards:
#define CONFIG_FLASH_AUTOPROTECT_LIST { {CFG_MONITOR_BASE, CFG_MONITOR_LEN} }
This looks fine and I will start voting for this, too.
;)
I am wondering if it is possible to get rid of the current autoprotection code (3 x flash_protect for monitor, env and 2nd env) in drivers/cfi_flash.c and fully implement it through the AUTOPROTECT_LIST without modifyiny all board config files. Any nice idea?
Not really. But it sounds like a good idea.
Especially on 4xx when you like to have sometinhg like
#define CONFIG_FLASH_AUTOPROTECT_LIST { {CFG_MONITOR_BASE, CFG_MONITOR_LEN} }
in your config file you will flash_protect most of the bootloader flash space twice.
Yes.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
participants (3)
-
Matthias Fuchs -
Stefan Roese -
Wolfgang Denk