[U-Boot] [PATCH 0/2] Protect splashimage from improperly aligned addresses

[U-Boot] [PATCH 0/2] Protect splashimage from improperly aligned addresses

[Nikita Kiryanov]

As discussed in the links below, one needs to be careful about choosing an
address for a splash image BMP file when working on architectures that can't
handle unaligned memory accesses. A bad address may lead to a bricked board,
and the safe addresses are not obvious due to the internal structure of BMP
files.

This patchset documents the problem and implements an optional callback that
prevents the environment variable from being set to a bad value.

Finally, it turns this protection on for cm_t35.

http://lists.denx.de/pipermail/u-boot/2013-January/144666.html
http://lists.denx.de/pipermail/u-boot/2013-February/146021.html

Nikita Kiryanov (2):
  lcd: implement a callback for splashimage
  cm_t35: prevent splashimage from being set to a bad value

 README                     |   11 +++++++++++
 common/lcd.c               |   26 ++++++++++++++++++++++++++
 doc/README.displaying-bmps |   27 +++++++++++++++++++++++++++
 include/configs/cm_t35.h   |    2 ++
 include/env_callback.h     |    7 +++++++
 5 files changed, 73 insertions(+)
 create mode 100644 doc/README.displaying-bmps

-- 
1.7.10.4

[U-Boot] [PATCH 1/2] lcd: implement a callback for splashimage

[Nikita Kiryanov]

On some architectures certain values of splashimage will lead to
a data abort exception.

Document the problem, and implement a callback for splashimage to
reject such values.

Cc: Anatolij Gustschin <agust@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
---

 README                     |   11 +++++++++++
 common/lcd.c               |   26 ++++++++++++++++++++++++++
 doc/README.displaying-bmps |   27 +++++++++++++++++++++++++++
 include/env_callback.h     |    7 +++++++
 4 files changed, 71 insertions(+)
 create mode 100644 doc/README.displaying-bmps

diff --git a/README b/README
index d8cb394..f1e416a 100644
--- a/README
+++ b/README
@@ -1530,6 +1530,17 @@ CBFS (Coreboot Filesystem) support
 		allows for a "silent" boot where a splash screen is
 		loaded very quickly after power-on.
 
+		CONFIG_SPLASHIMAGE_GUARD
+
+		If this option is set, then U-Boot will prevent the environment
+		variable "splashimage" from being set to a problematic address
+		(see README.displaying-bmps and README.arm-unaligned-accesses).
+		This option is useful for targets where, due to alignment
+		restrictions, an improperly aligned BMP image will cause a data
+		abort. If you don't think you will not have problems with
+		unaligned accesses (for example because your toolchain prevents
+		them) there is no need to set this option.
+
 		CONFIG_SPLASH_SCREEN_ALIGN
 
 		If this option is set the splash image can be freely positioned
diff --git a/common/lcd.c b/common/lcd.c
index 66d4f94..5d54168 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -33,6 +33,8 @@
 #include <common.h>
 #include <command.h>
 #include <stdarg.h>
+#include <search.h>
+#include <env_callback.h>
 #include <linux/types.h>
 #include <stdio_dev.h>
 #if defined(CONFIG_POST)
@@ -1084,6 +1086,30 @@ static void *lcd_logo(void)
 #endif /* CONFIG_LCD_LOGO && !CONFIG_LCD_INFO_BELOW_LOGO */
 }
 
+#ifdef CONFIG_SPLASHIMAGE_GUARD
+static int on_splashimage(const char *name, const char *value, enum env_op op,
+	int flags)
+{
+	ulong addr;
+	int aligned;
+
+	if (op == env_op_delete)
+		return 0;
+
+	addr = simple_strtoul(value, NULL, 16);
+	/* See README.displaying-bmps */
+	aligned = (addr % 4 == 2);
+	if (!aligned) {
+		printf("Invalid splashimage value. Value must be 16 bit aligned, but not 32 bit aligned\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+U_BOOT_ENV_CALLBACK(splashimage, on_splashimage);
+#endif
+
 void lcd_position_cursor(unsigned col, unsigned row)
 {
 	console_col = min(col, CONSOLE_COLS - 1);
diff --git a/doc/README.displaying-bmps b/doc/README.displaying-bmps
new file mode 100644
index 0000000..3311541
--- /dev/null
+++ b/doc/README.displaying-bmps
@@ -0,0 +1,27 @@
+If you are experiencing hangups/data-aborts when trying to display a BMP image,
+the following might be relevant to your situation...
+
+Some architectures cannot handle unaligned memory accesses, and an attempt to
+perform one will lead to a data abort. On such architectures it is necessary to
+make sure all data is properly aligned, and in many situations simply choosing
+a 32 bit aligned address is enough to ensure proper alignment. This is not
+always the case when dealing with data that has an internal layout such as a
+BMP image:
+
+BMP images have a header that starts with 2 byte-size fields followed by mostly
+32 bit fields. The packed struct that represents this header can be seen below:
+
+typedef struct bmp_header {
+	/* Header */
+	char signature[2];
+	__u32	file_size;
+	__u32	reserved;
+	__u32	data_offset;
+	... etc
+} __attribute__ ((packed)) bmp_header_t;
+
+When placed in an aligned address such as 0x80a00000, char signature offsets
+the __u32 fields into unaligned addresses (in our example 0x80a00002,
+0x80a00006, and so on...). When these fields are accessed by U-Boot, a 32 bit
+access is generated at a non-32-bit-aligned address, causing a data abort.
+The proper alignment for BMP images is therefore: 32-bit-aligned-address + 2.
diff --git a/include/env_callback.h b/include/env_callback.h
index c583120..62428d1 100644
--- a/include/env_callback.h
+++ b/include/env_callback.h
@@ -41,6 +41,12 @@
 #define SILENT_CALLBACK
 #endif
 
+#ifdef CONFIG_SPLASHIMAGE_GUARD
+#define SPLASHIMAGE_CALLBACK "splashimage:splashimage,"
+#else
+#define SPLASHIMAGE_CALLBACK
+#endif
+
 /*
  * This list of callback bindings is static, but may be overridden by defining
  * a new association in the ".callbacks" environment variable.
@@ -51,6 +57,7 @@
 	"bootfile:bootfile," \
 	"loadaddr:loadaddr," \
 	SILENT_CALLBACK \
+	SPLASHIMAGE_CALLBACK \
 	"stdin:console,stdout:console,stderr:console," \
 	CONFIG_ENV_CALLBACK_LIST_STATIC
 
-- 
1.7.10.4

[U-Boot] [PATCH 1/2] lcd: implement a callback for splashimage

[Wolfgang Denk]

Dear Nikita Kiryanov,

In message <1361722763-22953-2-git-send-email-nikita@compulab.co.il> you wrote:
> On some architectures certain values of splashimage will lead to
> a data abort exception.
> 
> Document the problem, and implement a callback for splashimage to
> reject such values.
> 
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
> 
>  README                     |   11 +++++++++++
>  common/lcd.c               |   26 ++++++++++++++++++++++++++
>  doc/README.displaying-bmps |   27 +++++++++++++++++++++++++++
>  include/env_callback.h     |    7 +++++++
>  4 files changed, 71 insertions(+)
>  create mode 100644 doc/README.displaying-bmps

Thanks.

Acked-by: Wolfgang Denk <wd@denx.de>

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
God made the integers; all else is the work of Man.       - Kronecker

[U-Boot] [PATCH V2 1/2] lcd: implement a callback for splashimage

[Nikita Kiryanov]

On some architectures certain values of splashimage will lead to
a data abort exception.

Document the problem, and implement a callback for splashimage to
reject such values.

Cc: Anatolij Gustschin <agust@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
---
Changes in V2:
	- A grammar correction in README:
	s/If you don't think you will not/If you think you will not

 README                     |   11 +++++++++++
 common/lcd.c               |   26 ++++++++++++++++++++++++++
 doc/README.displaying-bmps |   27 +++++++++++++++++++++++++++
 include/env_callback.h     |    7 +++++++
 4 files changed, 71 insertions(+)
 create mode 100644 doc/README.displaying-bmps

diff --git a/README b/README
index 4e4fd7d..f5bdab9 100644
--- a/README
+++ b/README
@@ -1530,6 +1530,17 @@ CBFS (Coreboot Filesystem) support
 		allows for a "silent" boot where a splash screen is
 		loaded very quickly after power-on.
 
+		CONFIG_SPLASHIMAGE_GUARD
+
+		If this option is set, then U-Boot will prevent the environment
+		variable "splashimage" from being set to a problematic address
+		(see README.displaying-bmps and README.arm-unaligned-accesses).
+		This option is useful for targets where, due to alignment
+		restrictions, an improperly aligned BMP image will cause a data
+		abort. If you think you will not have problems with unaligned
+		accesses (for example because your toolchain prevents them)
+		there is no need to set this option.
+
 		CONFIG_SPLASH_SCREEN_ALIGN
 
 		If this option is set the splash image can be freely positioned
diff --git a/common/lcd.c b/common/lcd.c
index ba6975b..590bbb9 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -33,6 +33,8 @@
 #include <common.h>
 #include <command.h>
 #include <stdarg.h>
+#include <search.h>
+#include <env_callback.h>
 #include <linux/types.h>
 #include <stdio_dev.h>
 #if defined(CONFIG_POST)
@@ -1099,6 +1101,30 @@ static void *lcd_logo(void)
 #endif /* CONFIG_LCD_LOGO && !CONFIG_LCD_INFO_BELOW_LOGO */
 }
 
+#ifdef CONFIG_SPLASHIMAGE_GUARD
+static int on_splashimage(const char *name, const char *value, enum env_op op,
+	int flags)
+{
+	ulong addr;
+	int aligned;
+
+	if (op == env_op_delete)
+		return 0;
+
+	addr = simple_strtoul(value, NULL, 16);
+	/* See README.displaying-bmps */
+	aligned = (addr % 4 == 2);
+	if (!aligned) {
+		printf("Invalid splashimage value. Value must be 16 bit aligned, but not 32 bit aligned\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+U_BOOT_ENV_CALLBACK(splashimage, on_splashimage);
+#endif
+
 void lcd_position_cursor(unsigned col, unsigned row)
 {
 	console_col = min(col, CONSOLE_COLS - 1);
diff --git a/doc/README.displaying-bmps b/doc/README.displaying-bmps
new file mode 100644
index 0000000..3311541
--- /dev/null
+++ b/doc/README.displaying-bmps
@@ -0,0 +1,27 @@
+If you are experiencing hangups/data-aborts when trying to display a BMP image,
+the following might be relevant to your situation...
+
+Some architectures cannot handle unaligned memory accesses, and an attempt to
+perform one will lead to a data abort. On such architectures it is necessary to
+make sure all data is properly aligned, and in many situations simply choosing
+a 32 bit aligned address is enough to ensure proper alignment. This is not
+always the case when dealing with data that has an internal layout such as a
+BMP image:
+
+BMP images have a header that starts with 2 byte-size fields followed by mostly
+32 bit fields. The packed struct that represents this header can be seen below:
+
+typedef struct bmp_header {
+	/* Header */
+	char signature[2];
+	__u32	file_size;
+	__u32	reserved;
+	__u32	data_offset;
+	... etc
+} __attribute__ ((packed)) bmp_header_t;
+
+When placed in an aligned address such as 0x80a00000, char signature offsets
+the __u32 fields into unaligned addresses (in our example 0x80a00002,
+0x80a00006, and so on...). When these fields are accessed by U-Boot, a 32 bit
+access is generated at a non-32-bit-aligned address, causing a data abort.
+The proper alignment for BMP images is therefore: 32-bit-aligned-address + 2.
diff --git a/include/env_callback.h b/include/env_callback.h
index c583120..62428d1 100644
--- a/include/env_callback.h
+++ b/include/env_callback.h
@@ -41,6 +41,12 @@
 #define SILENT_CALLBACK
 #endif
 
+#ifdef CONFIG_SPLASHIMAGE_GUARD
+#define SPLASHIMAGE_CALLBACK "splashimage:splashimage,"
+#else
+#define SPLASHIMAGE_CALLBACK
+#endif
+
 /*
  * This list of callback bindings is static, but may be overridden by defining
  * a new association in the ".callbacks" environment variable.
@@ -51,6 +57,7 @@
 	"bootfile:bootfile," \
 	"loadaddr:loadaddr," \
 	SILENT_CALLBACK \
+	SPLASHIMAGE_CALLBACK \
 	"stdin:console,stdout:console,stderr:console," \
 	CONFIG_ENV_CALLBACK_LIST_STATIC
 
-- 
1.7.10.4

[U-Boot] [PATCH 2/2] cm_t35: prevent splashimage from being set to a bad value

[Nikita Kiryanov]

Define CONFIG_SPLASHIMAGE_GUARD to prevent splashimage from being
set to a value that will cause U-Boot to hang while displaying a
splash screen.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
---

 include/configs/cm_t35.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/configs/cm_t35.h b/include/configs/cm_t35.h
index 943b658..dec94d2 100644
--- a/include/configs/cm_t35.h
+++ b/include/configs/cm_t35.h
@@ -331,6 +331,8 @@
 #define STATUS_LED_BOOT			STATUS_LED_BIT
 #define GREEN_LED_GPIO			186 /* CM-T35 Green LED is GPIO186 */
 
+#define CONFIG_SPLASHIMAGE_GUARD
+
 /* GPIO banks */
 #ifdef CONFIG_STATUS_LED
 #define CONFIG_OMAP3_GPIO_6	/* GPIO186 is in GPIO bank 6  */
-- 
1.7.10.4

[U-Boot] [PATCH 0/2] Protect splashimage from improperly aligned addresses

[Tom Rini]

On Sun, Feb 24, 2013 at 06:19:21PM +0200, Nikita Kiryanov wrote:

> As discussed in the links below, one needs to be careful about choosing an
> address for a splash image BMP file when working on architectures that can't
> handle unaligned memory accesses. A bad address may lead to a bricked board,
> and the safe addresses are not obvious due to the internal structure of BMP
> files.
> 
> This patchset documents the problem and implements an optional callback that
> prevents the environment variable from being set to a bad value.
> 
> Finally, it turns this protection on for cm_t35.
> 
> http://lists.denx.de/pipermail/u-boot/2013-January/144666.html
> http://lists.denx.de/pipermail/u-boot/2013-February/146021.html
> 
> Nikita Kiryanov (2):
>   lcd: implement a callback for splashimage
>   cm_t35: prevent splashimage from being set to a bad value

To be clear, this series is the only part required of all of the various
ones that have been posted about splash images?  Or are others still
needed here.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130227/7fab015d/attachment.pgp>

[U-Boot] [PATCH 0/2] Protect splashimage from improperly aligned addresses

[Nikita Kiryanov]

Hi Tom,

On 02/27/2013 10:06 PM, Tom Rini wrote:
> On Sun, Feb 24, 2013 at 06:19:21PM +0200, Nikita Kiryanov wrote:
>
>> As discussed in the links below, one needs to be careful about choosing an
>> address for a splash image BMP file when working on architectures that can't
>> handle unaligned memory accesses. A bad address may lead to a bricked board,
>> and the safe addresses are not obvious due to the internal structure of BMP
>> files.
>>
>> This patchset documents the problem and implements an optional callback that
>> prevents the environment variable from being set to a bad value.
>>
>> Finally, it turns this protection on for cm_t35.
>>
>> http://lists.denx.de/pipermail/u-boot/2013-January/144666.html
>> http://lists.denx.de/pipermail/u-boot/2013-February/146021.html
>>
>> Nikita Kiryanov (2):
>>    lcd: implement a callback for splashimage
>>    cm_t35: prevent splashimage from being set to a bad value
>
> To be clear, this series is the only part required of all of the various
> ones that have been posted about splash images?

Yes. It replaces "Create an API for safely accessing BMP header fields",
and I commented on the "Add splash screen for CM-T35" V3 series to point
out which patches should be dropped from it.

-- 
Regards,
Nikita.

[U-Boot] [PATCH 0/2] Protect splashimage from improperly aligned addresses

[Nikita Kiryanov]

Gentle ping.

On 02/24/2013 06:19 PM, Nikita Kiryanov wrote:
> As discussed in the links below, one needs to be careful about choosing an
> address for a splash image BMP file when working on architectures that can't
> handle unaligned memory accesses. A bad address may lead to a bricked board,
> and the safe addresses are not obvious due to the internal structure of BMP
> files.
>
> This patchset documents the problem and implements an optional callback that
> prevents the environment variable from being set to a bad value.
>
> Finally, it turns this protection on for cm_t35.
>
> http://lists.denx.de/pipermail/u-boot/2013-January/144666.html
> http://lists.denx.de/pipermail/u-boot/2013-February/146021.html
>
> Nikita Kiryanov (2):
>    lcd: implement a callback for splashimage
>    cm_t35: prevent splashimage from being set to a bad value
>
>   README                     |   11 +++++++++++
>   common/lcd.c               |   26 ++++++++++++++++++++++++++
>   doc/README.displaying-bmps |   27 +++++++++++++++++++++++++++
>   include/configs/cm_t35.h   |    2 ++
>   include/env_callback.h     |    7 +++++++
>   5 files changed, 73 insertions(+)
>   create mode 100644 doc/README.displaying-bmps
>


-- 
Regards,
Nikita.