flash hardware protection with CFI driver does not autoprotect monitor completely

U-Boot-Users Matthias Fuchs 3 participants 7 messages Nov 18, 2007 – Nov 19, 2007

Share & tools

Matthias Fuchs

author

2007-11-18 18:59 UTC #0 lore raw

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

Wolfgang Denk

2007-11-18 19:31 UTC #1 lore raw

Quoted 3 lines · In message <[email protected]> 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:

...

Quoted 1 line

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.

Quoted 8 lines

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

Signature · 5 lines

DENX Software Engineering GmbH,     MD: 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
In an organization, each person rises to the level of his own  incom-
petency                                         - The Peter Principle

Matthias Fuchs

author

2007-11-18 21:14 UTC #2 lore raw

Hi Wolfgang,

Quoted 9 lines · Wolfgang Denk

Quoted 3 lines · In message <[email protected]> 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:
...
Quoted 1 line
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:-)

Quoted 17 lines

Quoted 9 lines
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.

Quoted 6 lines

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:

drivers/cfi_flash.c +16 -0

index 5579a1e..0777349 100644
@@ -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

Wolfgang Denk

2007-11-18 21:52 UTC #3 lore raw

Dear Matthias,

Quoted 8 lines · in message <[email protected]> you wrote:

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

Quoted 4 lines

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?

Quoted 30 lines

And handle this list from the cfi driver:

drivers/cfi_flash.c +8 -0

index 5579a1e..0777349 100644
@@ -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)

drivers/cfi_flash.c unsigned long flash_init (void) · line 439

Quoted 10 lines

drivers/cfi_flash.c +8 -0

+       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

Signature · 5 lines

DENX Software Engineering GmbH,     MD: 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
Doubt isn't the opposite of faith; it is an element of faith.
- Paul Tillich, German theologian and historian

Stefan Roese

2007-11-19 06:14 UTC #4 lore raw

Quoted 9 lines · Wolfgang Denk

Quoted 9 lines
Quoted 6 lines
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?

Quoted 8 lines

Quoted 3 lines
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 at denx.de
=====================================================================

Matthias Fuchs

author

2007-11-19 10:04 UTC #5 lore raw

Hi,

Quoted 10 lines · Stefan Roese

Quoted 9 lines · Wolfgang Denk
Quoted 9 lines
Quoted 6 lines
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?

Quoted 8 lines

Quoted 8 lines
Quoted 3 lines
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.

Quoted 5 lines

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

Stefan Roese

2007-11-19 10:10 UTC #6 lore raw

Hi Matthias,

Quoted 7 lines · Matthias Fuchs

Quoted 5 lines
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.

;)

Quoted 4 lines

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.

Quoted 7 lines

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