[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Mark Tomlinson]

Our hardware has part of the flash mapped in two address ranges.
The CONFIG_SYS_MONITOR_BASE is in the upper 'boot' area, whereas
the CONFIG_SYS_FLASH_BANKS_LIST has the full flash available at
a lower address.

This all works fine until the code in cfi_flash.c:flash_init(), which
uses flash_get_info() to find the flash_info_t associated with the
monitor and environment. These are not in the probed flash range, so
flash_get_info() returns NULL. This is not checked and is passed
directly to flash_protect(). Since flash_protect() was not checking
for this NULL pointer either, random memory would be clobbered
causing the device to lock up.

This patch changes flash_protect() to check for the NULL, and the
error goes unreported.

Mark Tomlinson (1):
  flash: Check info pointer in flash_protect().

 common/flash.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)


NOTICE: This message contains privileged and confidential
information intended only for the use of the addressee
named above. If you are not the intended recipient of
this message you are hereby notified that you must not
disseminate, copy or take any action in reliance on it.
If you have received this message in error please
notify Allied Telesis Labs Ltd immediately.
Any views expressed in this message are those of the
individual sender, except where the sender has the
authority to issue and specifically states them to
be the views of Allied Telesis Labs.

[U-Boot] [PATCH 1/1] flash: Check info pointer in flash_protect().

[Mark Tomlinson]

Some calls to flash_protect() do not check that info is not
NULL. This change prevents this from causing random memory to
be stomped on.
---
 common/flash.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/common/flash.c b/common/flash.c
index eb4b2f5..dc376ee 100644
--- a/common/flash.c
+++ b/common/flash.c
@@ -43,15 +43,19 @@ extern flash_info_t  flash_info[]; /* info for FLASH chips */
 void
 flash_protect (int flag, ulong from, ulong to, flash_info_t *info)
 {
-	ulong b_end = info->start[0] + info->size - 1;	/* bank end address */
-	short s_end = info->sector_count - 1;	/* index of last sector */
+	ulong b_end;	/* bank end address */
+	short s_end;	/* index of last sector */
 	int i;
 
 	/* Do nothing if input data is bad. */
-	if (info->sector_count == 0 || info->size == 0 || to < from) {
+	if (!info || info->sector_count == 0 || info->size == 0 ||
+	    to < from) {
 		return;
 	}
 
+	b_end = info->start[0] + info->size - 1;
+	s_end = info->sector_count - 1;
+
 	debug ("flash_protect %s: from 0x%08lX to 0x%08lX\n",
 		(flag & FLAG_PROTECT_SET) ? "ON" :
 			(flag & FLAG_PROTECT_CLEAR) ? "OFF" : "???",
-- 
1.6.3


NOTICE: This message contains privileged and confidential
information intended only for the use of the addressee
named above. If you are not the intended recipient of
this message you are hereby notified that you must not
disseminate, copy or take any action in reliance on it.
If you have received this message in error please
notify Allied Telesis Labs Ltd immediately.
Any views expressed in this message are those of the
individual sender, except where the sender has the
authority to issue and specifically states them to
be the views of Allied Telesis Labs.

[U-Boot] [PATCH 1/1] flash: Check info pointer in flash_protect().

[Mike Frysinger]

On Tuesday 18 May 2010 01:26:35 Mark Tomlinson wrote:
> Some calls to flash_protect() do not check that info is not
> NULL. This change prevents this from causing random memory to
> be stomped on.
> ---

any patches need a s-o-b tag before they'll be accepted

> NOTICE: This message contains privileged and confidential

this junk has no business being on an open source mailing list
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100519/9283fcc0/attachment.pgp 

[U-Boot] [PATCH 1/1] flash: Check info pointer in flash_protect().

[Wolfgang Denk]

Dear Mark Tomlinson,

In message <1274160395-9308-2-git-send-email-mark.tomlinson@alliedtelesis.co.nz> you wrote:
> Some calls to flash_protect() do not check that info is not
> NULL. This change prevents this from causing random memory to
> be stomped on.

When exactly would that be the case?

> NOTICE: This message contains privileged and confidential
> information intended only for the use of the addressee
> named above. If you are not the intended recipient of
...

Such terms are obviously unacceptable for any code that is supposed to
go into mainline.

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
The C-shell doesn't parse. It adhoculates.
    - Casper.Dik at Holland.Sun.COM in <3ol96k$b2j@engnews2.Eng.Sun.COM>

[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Stefan Roese]

Hi Mark,

On Tuesday 18 May 2010 07:26:34 Mark Tomlinson wrote:
> Our hardware has part of the flash mapped in two address ranges.
> The CONFIG_SYS_MONITOR_BASE is in the upper 'boot' area, whereas
> the CONFIG_SYS_FLASH_BANKS_LIST has the full flash available at
> a lower address.

Just to be sure: You have 2 FLASH chips? Mapped at which addresses? And where 
does CONFIG_SYS_MONITOR_BASE point to?
 
> This all works fine until the code in cfi_flash.c:flash_init(), which
> uses flash_get_info() to find the flash_info_t associated with the
> monitor and environment. These are not in the probed flash range, so
> flash_get_info() returns NULL.

You mean that "flash_get_info(CONFIG_SYS_MONITOR_BASE)" returns NULL? Please 
explain again, why is this the case? 

> This is not checked and is passed
> directly to flash_protect(). Since flash_protect() was not checking
> for this NULL pointer either, random memory would be clobbered
> causing the device to lock up.
> 
> This patch changes flash_protect() to check for the NULL, and the
> error goes unreported.

I would prefer to fix the real problem, that flash_get_info() returns NULL, 
instead.
 
Cheers,
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] [PATCH 0/1] Fix hang trying to protect flash sectors

[mark tomlinson]

Yes we do have 2 flash chips. Here's the mapping: 

#define CONFIG_SYS_FLASH_BASE   0xf8000000  /* 2 chips*16M */ 
#define CONFIG_SYS_MONITOR_BASE  TEXT_BASE  /* start of monitor */ 

and in our config.mk file: 

TEXT_BASE = 0xfff40000 

This is the same flash chip as that at 0xf8000000, but remapped at reset by a CPLD to the high memory area too. 

The conditional code in cfi_flash.c: 
#if (CONFIG_SYS_MONITOR_BASE >= CONFIG_SYS_FLASH_BASE) && \ 
(!defined(CONFIG_MONITOR_IS_IN_RAM)) 
is therefore included since 0xfff40000 is greater than 0xf8000000, but flash_get_info(0xfff40000) returns NULL (as expected). 

I understand that not passing NULL to flash_protect() would be a better idea, and there's nothing wrong with doing both. I was going to fix it in cfi_flash.c, but noticed that many other areas of code (in different flash.c files) do the same thing. In our own build, I have just removed the code that tries to protect the monitor area, and will use an auto-protect area instead to do the same job. 

 - Mark 

>>> Stefan Roese <sr@denx.de> 5/18/2010 8:20 PM >>>
Hi Mark,

On Tuesday 18 May 2010 07:26:34 Mark Tomlinson wrote:
> Our hardware has part of the flash mapped in two address ranges.
> The CONFIG_SYS_MONITOR_BASE is in the upper 'boot' area, whereas
> the CONFIG_SYS_FLASH_BANKS_LIST has the full flash available at
> a lower address.

Just to be sure: You have 2 FLASH chips? Mapped at which addresses? And where
does CONFIG_SYS_MONITOR_BASE point to?

> This all works fine until the code in cfi_flash.c:flash_init(), which
> uses flash_get_info() to find the flash_info_t associated with the
> monitor and environment. These are not in the probed flash range, so
> flash_get_info() returns NULL.

You mean that "flash_get_info(CONFIG_SYS_MONITOR_BASE)" returns NULL? Please
explain again, why is this the case?

> This is not checked and is passed
> directly to flash_protect(). Since flash_protect() was not checking
> for this NULL pointer either, random memory would be clobbered
> causing the device to lock up.
>
> This patch changes flash_protect() to check for the NULL, and the
> error goes unreported.

I would prefer to fix the real problem, that flash_get_info() returns NULL,
instead.

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

NOTICE: This message contains privileged and confidential
information intended only for the use of the addressee
named above. If you are not the intended recipient of
this message you are hereby notified that you must not
disseminate, copy or take any action in reliance on it.
If you have received this message in error please
notify Allied Telesis Labs Ltd immediately.
Any views expressed in this message are those of the
individual sender, except where the sender has the
authority to issue and specifically states them to
be the views of Allied Telesis Labs.

[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Stefan Roese]

Mark,

On Tuesday 18 May 2010 22:10:51 mark tomlinson wrote:
> Yes we do have 2 flash chips. Here's the mapping:
> 
> #define CONFIG_SYS_FLASH_BASE   0xf8000000  /* 2 chips*16M */

Hmmm. 2 * 16MByte, thats 32MByte == 0x2000000. So you should have one chip 
at base address 0xff000000 and one at 0xfe000000. Why 0xf8000000?

> #define CONFIG_SYS_MONITOR_BASE  TEXT_BASE  /* start of monitor */
> 
> and in our config.mk file:
> 
> TEXT_BASE = 0xfff40000
> 
> This is the same flash chip as that at 0xf8000000, but remapped at reset 
by
> a CPLD to the high memory area too.

This seems wrong. See my comments above.
 
> The conditional code in cfi_flash.c:
> #if (CONFIG_SYS_MONITOR_BASE >= CONFIG_SYS_FLASH_BASE) && \
> (!defined(CONFIG_MONITOR_IS_IN_RAM))
> is therefore included since 0xfff40000 is greater than 0xf8000000, but
> flash_get_info(0xfff40000) returns NULL (as expected).

I don't see why flash_get_info(0xfff40000) should return NULL. It should 
return the pointer to the 16MB FLASH chip starting at 0xff000000.
 
> I understand that not passing NULL to flash_protect() would be a better
> idea, and there's nothing wrong with doing both.

Agreed in general. But we have to keep the code compact. And unnecessary 
checks do increase the code size (at least a small bit).

> I was going to fix it in
> cfi_flash.c, but noticed that many other areas of code (in different
> flash.c files) do the same thing. In our own build, I have just removed
> the code that tries to protect the monitor area, and will use an
> auto-protect area instead to do the same job.

"auto-protect area"? Please explain what you mean with this? Perhaps this 
is an interesting "feature" for mainline as well.

Cheers,
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] [PATCH 0/1] Fix hang trying to protect flash sectors

[mark tomlinson]

Sorry, no. The flash chips are 0xf8000000-0xf9ffffff (32MB). (Actually we have twice this area reserved for 64MB flash in the future). The flash chip from 0xf8000000 is also mirrored at 0xfff00000 at boot time. I don't know if you consider this a hardware bug to not have all the flash at the very top of memory, but that is the way our product has been designed. Here's a memory map of the high memory area: 

f8000000-fbffffff   64M Flash 
fe000000-fe0fffff    1M Battery-backed RAM 
ff000000-ff00ffff   64K On-board logic 
ff700000-ff7fffff    1M CCSR 
fff00000-ffffffff    1M Flash (mirror of f8000000). 

So we have CONFIG_SYS_FLASH_BASE pointing to the entire Flash, and CONFIG_SYS_MONITOR_BASE pointing to the mirrored section which contains u-boot. 

The auto protect was already there; nothing new. See CONFIG_SYS_FLASH_AUTOPROTECT_LIST in cfi_flash.c. 

>>> Stefan Roese <sr@denx.de> 5/19/2010 9:44 PM >>>
Mark,

On Tuesday 18 May 2010 22:10:51 mark tomlinson wrote:
> Yes we do have 2 flash chips. Here's the mapping:
>
> #define CONFIG_SYS_FLASH_BASE   0xf8000000  /* 2 chips*16M */

Hmmm. 2 * 16MByte, thats 32MByte == 0x2000000. So you should have one chip
at base address 0xff000000 and one at 0xfe000000. Why 0xf8000000?

> #define CONFIG_SYS_MONITOR_BASE  TEXT_BASE  /* start of monitor */
>
> and in our config.mk file:
>
> TEXT_BASE = 0xfff40000
>
> This is the same flash chip as that at 0xf8000000, but remapped at reset
by
> a CPLD to the high memory area too.

This seems wrong. See my comments above.

> The conditional code in cfi_flash.c:
> #if (CONFIG_SYS_MONITOR_BASE >= CONFIG_SYS_FLASH_BASE) && \
> (!defined(CONFIG_MONITOR_IS_IN_RAM))
> is therefore included since 0xfff40000 is greater than 0xf8000000, but
> flash_get_info(0xfff40000) returns NULL (as expected).

I don't see why flash_get_info(0xfff40000) should return NULL. It should
return the pointer to the 16MB FLASH chip starting at 0xff000000.

> I understand that not passing NULL to flash_protect() would be a better
> idea, and there's nothing wrong with doing both.

Agreed in general. But we have to keep the code compact. And unnecessary
checks do increase the code size (at least a small bit).

> I was going to fix it in
> cfi_flash.c, but noticed that many other areas of code (in different
> flash.c files) do the same thing. In our own build, I have just removed
> the code that tries to protect the monitor area, and will use an
> auto-protect area instead to do the same job.

"auto-protect area"? Please explain what you mean with this? Perhaps this
is an interesting "feature" for mainline as well.

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

NOTICE: This message contains privileged and confidential
information intended only for the use of the addressee
named above. If you are not the intended recipient of
this message you are hereby notified that you must not
disseminate, copy or take any action in reliance on it.
If you have received this message in error please
notify Allied Telesis Labs Ltd immediately.
Any views expressed in this message are those of the
individual sender, except where the sender has the
authority to issue and specifically states them to
be the views of Allied Telesis Labs.

[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Wolfgang Denk]

Dear "mark tomlinson",


Please restrict your line length to some 70 characters or so, and stop
posting HTML!

In message <4BF4FC5E020000B80001201A@gwia.alliedtelesyn.co.nz> you wrote:
>
> Sorry, no. The flash chips are 0xf8000000-0xf9ffffff (32MB). (Actually we
>  have twice this area reserved for 64MB flash in the future). The flash
>  chip from 0xf8000000 is also mirrored at 0xfff00000 at boot time. I do
> n't know if you consider this a hardware bug to not have all the flash at
>  the very top of memory, but that is the way our product has been desig
> ned. Here's a memory map of the high memory area: 

It's not a hardware bug, but a configuration error.

> f8000000-fbffffff   64M Flash 
> fe000000-fe0fffff    1M Battery-backed RAM 
> ff000000-ff00ffff   64K On-board logic 
> ff700000-ff7fffff    1M CCSR 
> fff00000-ffffffff    1M Flash (mirror of f8000000). 

This makes no sense. Fix your memory map, and map the flash (all of
it) to the end of the address space.

> So we have CONFIG_SYS_FLASH_BASE pointing to the entire Flash, and CONFIG
> _SYS_MONITOR_BASE pointing to the mirrored section which contains u-boot.

As I mentioned above: misconigured.

> NOTICE: This message contains privileged and confidential
> information intended only for the use of the addressee
...

And please stop posting these silly disclaimers.

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
Insufficient facts always invite danger.
	-- Spock, "Space Seed", stardate 3141.9

[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Chris Packham]

Disclaimer: I'm workmate of Mark's

Wolfgang Denk <wd <at> denx.de> writes:
>
> It's not a hardware bug, but a configuration error.
> 
> > f8000000-fbffffff   64M Flash 
> > fe000000-fe0fffff    1M Battery-backed RAM 
> > ff000000-ff00ffff   64K On-board logic 
> > ff700000-ff7fffff    1M CCSR 
> > fff00000-ffffffff    1M Flash (mirror of f8000000). 
> 
> This makes no sense. Fix your memory map, and map the flash (all of
> it) to the end of the address space.

While it would be possible to shuffle the memory map around there is one
problem with the hardware design that I don't think can be overcome (I'd
love to be proven wrong). The boot chip select is mapped to the _bottom_
of the first flash chip. It was done this way so that we could expand the
flash in the future as a rolling production change (we're now shipping
units with 64MB fitted). i.e. we knew we could rely on a fixed base
address so thats where we pointed the boot chip select.

I think in hindsight we could have modified our flash detection code to
start at the top and jump backwards looking for extra chips. Unfortunately
we're not able to change the hardware design for this product but we can
take this into account on future designs.

> > NOTICE: This message contains privileged and confidential
> > information intended only for the use of the addressee
> ...
> 
> And please stop posting these silly disclaimers.
> 

Corporate overlords have been flogged :). General response has been to go
sign up to gmail. Trust me it annoys us as much as it annoys you.

- C

[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Wolfgang Denk]

Dear Chris Packham,

In message <loom.20100520T005209-871@post.gmane.org> you wrote:
> 
> While it would be possible to shuffle the memory map around there is one
> problem with the hardware design that I don't think can be overcome (I'd
> love to be proven wrong). The boot chip select is mapped to the _bottom_
> of the first flash chip. It was done this way so that we could expand the
> flash in the future as a rolling production change (we're now shipping
> units with 64MB fitted). i.e. we knew we could rely on a fixed base
> address so thats where we pointed the boot chip select.

I don't see why this should be relevant. Usually you can set up nearly
any memory map in software, independent of the CPU state at boot time.

Which exact processor family are we talking about?

> I think in hindsight we could have modified our flash detection code to
> start at the top and jump backwards looking for extra chips. Unfortunately

What do you mean by "our flash detection code"? Are you using any
private code for that? Why? U-Boot already has all the needed stuff,
just use it.

> we're not able to change the hardware design for this product but we can
> take this into account on future designs.

I'm not convinced that any hardware changes would be needed.

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
Don't panic.

[U-Boot] [PATCH 0/1] Fix hang trying to protect flash sectors

[Chris Packham]

Hi Wolfgang,

On Thu, May 20, 2010 at 1:35 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Chris Packham,
>
> In message <loom.20100520T005209-871@post.gmane.org> you wrote:
>>
>> While it would be possible to shuffle the memory map around there is one
>> problem with the hardware design that I don't think can be overcome (I'd
>> love to be proven wrong). The boot chip select is mapped to the _bottom_
>> of the first flash chip. It was done this way so that we could expand the
>> flash in the future as a rolling production change (we're now shipping
>> units with 64MB fitted). i.e. we knew we could rely on a fixed base
>> address so thats where we pointed the boot chip select.
>
> I don't see why this should be relevant. Usually you can set up nearly
> any memory map in software, independent of the CPU state at boot time.
>
> Which exact processor family are we talking about?

Freescale MPC8541. CS0 is mapped by an external CPLD to either the
bottom block of flash _or_ to a plug in card which has physical EPROMs
which we use when we're bringing the board up. On newer designs we
actually use pre-programmed flash chips in the factory and JTAG in
circuit debuggers for development.

>> I think in hindsight we could have modified our flash detection code to
>> start at the top and jump backwards looking for extra chips. Unfortunately
>
> What do you mean by "our flash detection code"? Are you using any
> private code for that? Why? U-Boot already has all the needed stuff,
> just use it.
>

This design was originally done for a proprietary boot loader before
we saw the light and switched to u-boot. That was what I meant by "our
flash detection code".

Recently we've just switched to using the CFI driver in the latest
u-boot version. Prior to this (based on version 1.1.4) we still had
our own board/.../flash.c.

>> we're not able to change the hardware design for this product but we can
>> take this into account on future designs.
>
> I'm not convinced that any hardware changes would be needed.

I think we'd need 2 different boot loaders configurations, one for
running from EPROM, one for running from flash.

If we ignore the EPROM it wouldn't be too hard to re-define the memory
map such that we have flash arranged with the boot loader and
environment in the bottom 1MB of the first flash chip with the file
system in the top 31MB and optionally the second flash chip. The plug
in EPROM card is still very useful when you accidentally brick your
board so we'd still want to have that, which could be done easily with
the existing configuration.

I'll look into shuffling the memory map around. It'd also be a good
opportunity to start using the mtd maps in Linux.

- C