Patchwork [U-Boot,V2,3/7] lcd: prevent unaligned memory access when displaying splash screen

login
register
mail settings
Submitter Nikita Kiryanov
Date Jan. 29, 2013, 12:42 p.m.
Message ID <1359463349-11649-4-git-send-email-nikita@compulab.co.il>
Download mbox | patch
Permalink /patch/216519/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Nikita Kiryanov - Jan. 29, 2013, 12:42 p.m.
When the bmp file is loaded to an address specified by the environment
variable "splashimage", its header members might be unaligned.
This happens because the bmp header starts with two byte-size fields followd by
mostly 32 bit fields, so when the address in splashimage is not equal to aligned
address plus/minus 2, the 32 bit members will be placed in unaligned addresses.
This results in a data abort on targets that cannot handle unaligned memory
accesses.

Check that the address is safe to use, and fix it if it's not.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
NOTE: New patch in the series; no V1.

 common/lcd.c |    8 ++++++++
 1 file changed, 8 insertions(+)
Wolfgang Denk - Jan. 29, 2013, 1:46 p.m.
Dear Nikita Kiryanov,

In message <1359463349-11649-4-git-send-email-nikita@compulab.co.il> you wrote:
> When the bmp file is loaded to an address specified by the environment
> variable "splashimage", its header members might be unaligned.
> This happens because the bmp header starts with two byte-size fields followd by
> mostly 32 bit fields, so when the address in splashimage is not equal to aligned
> address plus/minus 2, the 32 bit members will be placed in unaligned addresses.
> This results in a data abort on targets that cannot handle unaligned memory
> accesses.

I don;t understand your +/- 2 argument.  When there are 32 bit fielkds
in the header, these have to be 32 bit aligned, right?

> Check that the address is safe to use, and fix it if it's not.

If you perform any such fixes (thus essentially not doing what the
user asked for), you should at least print a warning message so the
user gets aware of such issues.

Also, you should add the video custodian on Cc: for such patches.

[Done here].


Best regards,

Wolfgang Denk
Nikita Kiryanov - Jan. 29, 2013, 2:34 p.m.
Hi Wolfgang Denk,

On 01/29/2013 03:46 PM, Wolfgang Denk wrote:
> Dear Nikita Kiryanov,
>
> In message <1359463349-11649-4-git-send-email-nikita@compulab.co.il> you wrote:
>> When the bmp file is loaded to an address specified by the environment
>> variable "splashimage", its header members might be unaligned.
>> This happens because the bmp header starts with two byte-size fields followd by
>> mostly 32 bit fields, so when the address in splashimage is not equal to aligned
>> address plus/minus 2, the 32 bit members will be placed in unaligned addresses.
>> This results in a data abort on targets that cannot handle unaligned memory
>> accesses.
>
> I don;t understand your +/- 2 argument.  When there are 32 bit fielkds
> in the header, these have to be 32 bit aligned, right?

Right. The problem stems from the structure of the bmp header, which
places the 32 bit fields at an offset of 2 from the load address.

 From bmp_layout.h:
typedef struct bmp_header {
	/* Header */
	char signature[2];   <--- This is what's causing the offset
	__u32	file_size;
	__u32	reserved;
	__u32	data_offset;
...
}

This means that in order for these 32 bit fields to be properly aligned
when you load the bmp into memory, it has to be placed with a +/- offset
relative to an aligned address to cancel the shift that
char signature[2] causes.

>
>> Check that the address is safe to use, and fix it if it's not.
>
> If you perform any such fixes (thus essentially not doing what the
> user asked for), you should at least print a warning message so the
> user gets aware of such issues.
>
> Also, you should add the video custodian on Cc: for such patches.
>
> [Done here].

Thanks, I'll do that in the future.

>
>
> Best regards,
>
> Wolfgang Denk
>

Patch

diff --git a/common/lcd.c b/common/lcd.c
index 66d4f94..104125d 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -1046,6 +1046,14 @@  static void *lcd_logo(void)
 		do_splash = 0;
 
 		addr = simple_strtoul (s, NULL, 16);
+		/*
+		 * In order for the fields of bmp header to be properly aligned
+		 * in memory, splash image addr must be aligned to "aligned
+		 * address plus 2". Fix addr if necessary.
+		 */
+		if (addr % 4 != 2)
+			addr += (addr % 4) ?: 2;
+
 #ifdef CONFIG_SPLASH_SCREEN_ALIGN
 		s = getenv("splashpos");
 		if (s != NULL) {