[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[Matthias Fuchs]

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

[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[Wolfgang Denk]

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

-- 
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

[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[Matthias Fuchs]

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

[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[Wolfgang Denk]

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

-- 
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

[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[Stefan Roese]

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

[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[Matthias Fuchs]

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

[U-Boot-Users] flash hardware protection with CFI driver does not autoprotect monitor completely

[Stefan Roese]

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