From mboxrd@z Thu Jan  1 00:00:00 1970
From: Matthias Fuchs <matthias.fuchs@esd-electronics.com>
Date: Sun, 18 Nov 2007 22:14:05 +0100
Subject: [U-Boot-Users] flash hardware protection with CFI driver does
	not autoprotect monitor completely
In-Reply-To: <20071118193105.53B58243A9@gemini.denx.de>
References: <20071118193105.53B58243A9@gemini.denx.de>
Message-ID: <200711182214.06137.matthias.fuchs@esd-electronics.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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