PS3: Fix printing of os-area magic numbers

PATCH Superseded Geoff Levand 3 participants 9 messages Nov 17, 2007 – Dec 23, 2007

Subsystems (2)

PowerPC PS3 platform support

lore

Share & tools

lore

Geoff Levand

author

2007-11-17 22:26 UTC #0 lore raw

PATCH 1/1 PS3: Fix printing of os-area magic numbers

Fix a bug in the printing of the PS3 os-area magic numbers which assumed that magic numbers were zero terminated strings. The magic numbers are represented in memory as integers. If the os-area sections are not initialized correctly they could contained random data that would be printed to the display.

CC Geert Uytterhoeven <[email protected]>

Signed-off-by Geoff Levand <[email protected]>

---

Paul,

This fixes a very minor bug in linus' current tree. Please consider for 2.6.24.

-Geoff

 arch/powerpc/platforms/ps3/os-area.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

arch/powerpc/platforms/ps3/os-area.c +12 -2

@@ -269,8 +269,13 @@ static void __init os_area_get_property(
 static void _dump_header(const struct os_area_header *h, const char *func,
 	int line)
 {
+	u8 str[sizeof(h->magic_num) + 1];
+
+	memcpy(str, h->magic_num, sizeof(h->magic_num));
+	str[sizeof(h->magic_num)] = 0;
+
 	pr_debug("%s:%d: h.magic_num:       '%s'\n", func, line,
-		h->magic_num);
+		str);
 	pr_debug("%s:%d: h.hdr_version:     %u\n", func, line,
 		h->hdr_version);
 	pr_debug("%s:%d: h.db_area_offset:  %u\n", func, line,

@@ -484,8 +489,13 @@ static int db_get_rtc_diff(const struct 
 static void _dump_db(const struct os_area_db *db, const char *func,
 	int line)
 {
+	u8 str[sizeof(db->magic_num) + 1];
+
+	memcpy(str, &db->magic_num, sizeof(db->magic_num));
+	str[sizeof(db->magic_num)] = 0;
+
 	pr_debug("%s:%d: db.magic_num:      '%s'\n", func, line,
-		(const char*)&db->magic_num);
+		str);
 	pr_debug("%s:%d: db.version:         %u\n", func, line,
 		db->version);
 	pr_debug("%s:%d: db.index_64:        %u\n", func, line,

Geert Uytterhoeven

2007-11-18 09:33 UTC #1 lore raw

Quoted 23 lines · Geoff Levand

Fix a bug in the printing of the PS3 os-area magic numbers which assumed that magic numbers were zero terminated strings. The magic numbers are represented in memory as integers. If the os-area sections are not initialized correctly they could contained random data that would be printed to the display.
CC Geert Uytterhoeven <[email protected]>
Signed-off-by Geoff Levand <[email protected]>
---
Paul,
This fixes a very minor bug in linus' current tree. Please consider for 2.6.24.
(quoted diff) +0 -1
-Geoff
 
 arch/powerpc/platforms/ps3/os-area.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
arch/powerpc/platforms/ps3/os-area.c +0 -0
@@ -269,8 +269,13 @@ static void __init os_area_get_property(
 static void _dump_header(const struct os_area_header *h, const char *func,

arch/powerpc/platforms/ps3/os-area.c static void __init os_area_get_property( · line 278

Quoted 10 lines

arch/powerpc/platforms/ps3/os-area.c +6 -1

 	int line)
 {
+	u8 str[sizeof(h->magic_num) + 1];
+
+	memcpy(str, h->magic_num, sizeof(h->magic_num));
+	str[sizeof(h->magic_num)] = 0;
+
 	pr_debug("%s:%d: h.magic_num:       '%s'\n", func, line,
-		h->magic_num);
+		str);

This only fixes the problem of a missing zero-termination (which could be handled using `%*s').

I'm also worried about unprintable characters.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   [email protected]	
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V. Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Geoff Levand

author

2007-11-22 22:09 UTC #2 lore raw

PATCH v2 1/1 PS3: Fix printing of os-area magic numbers

In reply to Geoff Levand

Fix a bug in the printing of the os-area magic numbers which assumed that magic numbers were zero terminated strings. The magic numbers are represented in memory as integers. If the os-area sections are not initialized correctly they could contained random data that would be printed to the display.

CC Geert Uytterhoeven <[email protected]>

Signed-off-by Geoff Levand <[email protected]>

---
v2: handle unprintable chars.

Paul,

This fixes a very minor bug in linus' current tree. Please consider for 2.6.24.

-Geoff

 arch/powerpc/platforms/ps3/os-area.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

arch/powerpc/platforms/ps3/os-area.c +21 -2

@@ -23,6 +23,7 @@
 #include <linux/workqueue.h>
 #include <linux/fs.h>
 #include <linux/syscalls.h>
+#include <linux/ctype.h>
 
 #include <asm/lmb.h>

@@ -269,8 +270,17 @@ static void __init os_area_get_property(
 static void _dump_header(const struct os_area_header *h, const char *func,
 	int line)
 {
+	u8 str[sizeof(h->magic_num) + 1];
+	u8 *s, *d;
+
+	for(s = h->magic_num, d = str; s < h->magic_num + sizeof(h->magic_num);
+		s++, d++) {
+		*d = isprint(*s) ? *s : '.';
+	}
+	d[sizeof(h->magic_num)] = 0;
+
 	pr_debug("%s:%d: h.magic_num:       '%s'\n", func, line,
-		h->magic_num);
+		str);
 	pr_debug("%s:%d: h.hdr_version:     %u\n", func, line,
 		h->hdr_version);
 	pr_debug("%s:%d: h.db_area_offset:  %u\n", func, line,

@@ -484,8 +494,17 @@ static int db_get_rtc_diff(const struct 
 static void _dump_db(const struct os_area_db *db, const char *func,
 	int line)
 {
+	u8 str[sizeof(db->magic_num) + 1];
+	u8 *s, *d;
+
+	for(s = (u8*)&db->magic_num, d = str;
+		s < (u8*)&db->magic_num + sizeof(db->magic_num); s++, d++) {
+		*d = isprint(*s) ? *s : '.';
+	}
+	d[sizeof(db->magic_num)] = 0;
+
 	pr_debug("%s:%d: db.magic_num:      '%s'\n", func, line,
-		(const char*)&db->magic_num);
+		str);
 	pr_debug("%s:%d: db.version:         %u\n", func, line,
 		db->version);
 	pr_debug("%s:%d: db.index_64:        %u\n", func, line,

Paul Mackerras

2007-12-21 04:44 UTC #3 lore raw

arch/powerpc/platforms/ps3/os-area.c static void __init os_area_get_property( · line 280

Quoted 13 lines · Geoff Levand

Fix a bug in the printing of the os-area magic numbers which assumed that magic numbers were zero terminated strings. The magic numbers are represented in memory as integers. If the os-area sections are not initialized correctly they could contained random data that would be printed to the display.
arch/powerpc/platforms/ps3/os-area.c +8 -0
+	u8 str[sizeof(h->magic_num) + 1];
+	u8 *s, *d;
+
+	for(s = h->magic_num, d = str; s < h->magic_num + sizeof(h->magic_num);
+		s++, d++) {
+		*d = isprint(*s) ? *s : '.';
+	}
+	d[sizeof(h->magic_num)] = 0;

This last statement is wrong, because d has been incremented to point to the last byte of str already by this stage.

It would be nicer if you pulled out the two instances of the for loop into a little helper function.

arch/powerpc/platforms/ps3/os-area.c static int db_get_rtc_diff(const struct · line 500

Quoted 1 line

arch/powerpc/platforms/ps3/os-area.c +1 -0
+	for(s = (u8*)&db->magic_num, d = str;

Why do you need the (u8*) cast in this case but not the other?

Paul.

Geoff Levand

author

2007-12-23 04:54 UTC #4 lore raw

Quoted 19 lines · Paul Mackerras

arch/powerpc/platforms/ps3/os-area.c static void __init os_area_get_property( · line 280
Quoted 13 lines · Geoff Levand
Fix a bug in the printing of the os-area magic numbers which assumed that magic numbers were zero terminated strings. The magic numbers are represented in memory as integers. If the os-area sections are not initialized correctly they could contained random data that would be printed to the display.
arch/powerpc/platforms/ps3/os-area.c +8 -0
+	u8 str[sizeof(h->magic_num) + 1];
+	u8 *s, *d;
+
+	for(s = h->magic_num, d = str; s < h->magic_num + sizeof(h->magic_num);
+		s++, d++) {
+		*d = isprint(*s) ? *s : '.';
+	}
+	d[sizeof(h->magic_num)] = 0;
This last statement is wrong, because d has been incremented to point to the last byte of str already by this stage.

Quoted 2 lines

It would be nicer if you pulled out the two instances of the for loop into a little helper function.

OK.

Quoted 3 lines

arch/powerpc/platforms/ps3/os-area.c static int db_get_rtc_diff(const struct · line 500
Quoted 1 line
arch/powerpc/platforms/ps3/os-area.c +1 -0
+	for(s = (u8*)&db->magic_num, d = str;
Why do you need the (u8*) cast in this case but not the other?

The types are different. The header magic is an array of u8, the db magic is a u32.

-Geoff

Geoff Levand

author

2007-12-23 05:10 UTC #5 lore raw

PATCH v3 1/1 PS3: Fix printing of os-area magic numbers

In reply to Geoff Levand

CC Geert Uytterhoeven <[email protected]>

Signed-off-by Geoff Levand <[email protected]>

---
v2: o handle unprintable chars.

v3: o breakout string dump into helper dump_field()
o unify handling of header and db magic numbers

 arch/powerpc/platforms/ps3/os-area.c |   40 ++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 10 deletions(-)

arch/powerpc/platforms/ps3/os-area.c +30 ~~-10~~

@@ -23,6 +23,7 @@
 #include <linux/workqueue.h>
 #include <linux/fs.h>
 #include <linux/syscalls.h>
+#include <linux/ctype.h>
 
 #include <asm/lmb.h>

@@ -37,6 +38,8 @@ enum os_area_ldr_format {
 	HEADER_LDR_FORMAT_GZIP = 1,
 };
 
+#define OS_AREA_HEADER_MAGIC_NUM "cell_ext_os_area"
+
 /**
  * struct os_area_header - os area header segment.
  * @magic_num: Always 'cell_ext_os_area'.

@@ -114,13 +117,11 @@ struct os_area_params {
 	u8 _reserved_5[8];
 };
 
-enum {
-	OS_AREA_DB_MAGIC_NUM = 0x2d64622dU,
-};
+#define OS_AREA_DB_MAGIC_NUM "-db-"
 
 /**
  * struct os_area_db - Shared flash memory database.
- * @magic_num: Always '-db-' = 0x2d64622d.
+ * @magic_num: Always '-db-'.
  * @version: os_area_db format version number.
  * @index_64: byte offset of the database id index for 64 bit variables.
  * @count_64: number of usable 64 bit index entries

@@ -135,7 +136,7 @@ enum {
  */
 
 struct os_area_db {
-	u32 magic_num;
+	u8 magic_num[4];
 	u16 version;
 	u16 _reserved_1;
 	u16 index_64;

@@ -265,12 +266,26 @@ static void __init os_area_get_property(
 			prop->name);
 }
 
+static void dump_field(char *s, const u8 *f, unsigned int size)
+{
+#if defined(DEBUG)
+	unsigned int i;
+
+	for (i = 0; i < size; i++)
+		s[i] = isprint(f[i]) ? f[i] : '.';
+	s[i] = 0;
+#endif
+}
+
 #define dump_header(_a) _dump_header(_a, __func__, __LINE__)
 static void _dump_header(const struct os_area_header *h, const char *func,
 	int line)
 {
+	char str[sizeof(h->magic_num) + 1];
+
+	dump_field(str, h->magic_num, sizeof(h->magic_num));
 	pr_debug("%s:%d: h.magic_num:       '%s'\n", func, line,
-		h->magic_num);
+		str);
 	pr_debug("%s:%d: h.hdr_version:     %u\n", func, line,
 		h->hdr_version);
 	pr_debug("%s:%d: h.db_area_offset:  %u\n", func, line,

@@ -311,7 +326,8 @@ static void _dump_params(const struct os
 
 static int verify_header(const struct os_area_header *header)
 {
-	if (memcmp(header->magic_num, "cell_ext_os_area", 16)) {
+	if (memcmp(header->magic_num, OS_AREA_HEADER_MAGIC_NUM,
+		sizeof(header->magic_num))) {
 		pr_debug("%s:%d magic_num failed\n", __func__, __LINE__);
 		return -1;
 	}

@@ -331,7 +347,8 @@ static int verify_header(const struct os
 
 static int db_verify(const struct os_area_db *db)
 {
-	if (db->magic_num != OS_AREA_DB_MAGIC_NUM) {
+	if (memcmp(db->magic_num, OS_AREA_DB_MAGIC_NUM,
+		sizeof(db->magic_num))) {
 		pr_debug("%s:%d magic_num failed\n", __func__, __LINE__);
 		return -1;
 	}

@@ -484,8 +501,11 @@ static int db_get_rtc_diff(const struct 
 static void _dump_db(const struct os_area_db *db, const char *func,
 	int line)
 {
+	char str[sizeof(db->magic_num) + 1];
+
+	dump_field(str, db->magic_num, sizeof(db->magic_num));
 	pr_debug("%s:%d: db.magic_num:      '%s'\n", func, line,
-		(const char*)&db->magic_num);
+		str);
 	pr_debug("%s:%d: db.version:         %u\n", func, line,
 		db->version);
 	pr_debug("%s:%d: db.index_64:        %u\n", func, line,

@@ -516,7 +536,7 @@ static void os_area_db_init(struct os_ar
 
 	memset(db, 0, sizeof(struct os_area_db));
 
-	db->magic_num = OS_AREA_DB_MAGIC_NUM;
+	memcpy(db->magic_num, OS_AREA_DB_MAGIC_NUM, sizeof(db->magic_num));
 	db->version = 1;
 	db->index_64 = HEADER_SIZE;
 	db->count_64 = VALUES_64_COUNT;

Geert Uytterhoeven

2007-12-23 09:51 UTC #6 lore raw

arch/powerpc/platforms/ps3/os-area.c static void __init os_area_get_property( · line 278

Quoted 10 lines · Geoff Levand

arch/powerpc/platforms/ps3/os-area.c +10 -0

+static void dump_field(char *s, const u8 *f, unsigned int size)
+{
+#if defined(DEBUG)
+	unsigned int i;
+
+	for (i = 0; i < size; i++)
+		s[i] = isprint(f[i]) ? f[i] : '.';
+	s[i] = 0;
+#endif
+}

Sorry for nitpicking again.

Usually the _length_ of a C-string is the number of characters, while the _size_ of a C-string includes the zero-terminator.

In dump_field() it writes size+1 bytes to s.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   [email protected]	
Internet: http://www.sony-europe.com/

Geoff Levand

author

2007-12-23 17:34 UTC #7 lore raw

Quoted 14 lines · Geert Uytterhoeven

arch/powerpc/platforms/ps3/os-area.c static void __init os_area_get_property( · line 278
Quoted 10 lines · Geoff Levand
arch/powerpc/platforms/ps3/os-area.c +10 -0
+static void dump_field(char *s, const u8 *f, unsigned int size)
+{
+#if defined(DEBUG)
+	unsigned int i;
+
+	for (i = 0; i < size; i++)
+		s[i] = isprint(f[i]) ? f[i] : '.';
+	s[i] = 0;
+#endif
+}
Sorry for nitpicking again.

Quoted 4 lines

Usually the _length_ of a C-string is the number of characters, while the _size_ of a C-string includes the zero-terminator.
In dump_field() it writes size+1 bytes to s.

The arg size is the size of the field in bytes:

  dump_field(str, h->magic_num, sizeof(h->magic_num));

To me, the name length doesn't seem to convey that.

-Geoff

Geoff Levand

author

2007-12-23 18:24 UTC #8 lore raw

PATCH v4 1/1 PS3: Fix printing of os-area magic numbers

In reply to Geoff Levand

Subject: ps3: Fix printing of os-area magic numbers

CC Geert Uytterhoeven <[email protected]>

Signed-off-by Geoff Levand <[email protected]>

---
v2: o handle unprintable chars.

v3: o breakout string dump into helper dump_field()
o unify handling of header and db magic numbers

v4: rename dump_field() args

 arch/powerpc/platforms/ps3/os-area.c |   40 ++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 10 deletions(-)

arch/powerpc/platforms/ps3/os-area.c +30 ~~-10~~

@@ -23,6 +23,7 @@
 #include <linux/workqueue.h>
 #include <linux/fs.h>
 #include <linux/syscalls.h>
+#include <linux/ctype.h>
 
 #include <asm/lmb.h>

@@ -37,6 +38,8 @@ enum os_area_ldr_format {
 	HEADER_LDR_FORMAT_GZIP = 1,
 };
 
+#define OS_AREA_HEADER_MAGIC_NUM "cell_ext_os_area"
+
 /**
  * struct os_area_header - os area header segment.
  * @magic_num: Always 'cell_ext_os_area'.

@@ -114,13 +117,11 @@ struct os_area_params {
 	u8 _reserved_5[8];
 };
 
-enum {
-	OS_AREA_DB_MAGIC_NUM = 0x2d64622dU,
-};
+#define OS_AREA_DB_MAGIC_NUM "-db-"
 
 /**
  * struct os_area_db - Shared flash memory database.
- * @magic_num: Always '-db-' = 0x2d64622d.
+ * @magic_num: Always '-db-'.
  * @version: os_area_db format version number.
  * @index_64: byte offset of the database id index for 64 bit variables.
  * @count_64: number of usable 64 bit index entries

@@ -135,7 +136,7 @@ enum {
  */
 
 struct os_area_db {
-	u32 magic_num;
+	u8 magic_num[4];
 	u16 version;
 	u16 _reserved_1;
 	u16 index_64;

@@ -265,12 +266,26 @@ static void __init os_area_get_property(
 			prop->name);
 }
 
+static void dump_field(char *s, const u8 *field, int size_of_field)
+{
+#if defined(DEBUG)
+	int i;
+
+	for (i = 0; i < size_of_field; i++)
+		s[i] = isprint(field[i]) ? field[i] : '.';
+	s[i] = 0;
+#endif
+}
+
 #define dump_header(_a) _dump_header(_a, __func__, __LINE__)
 static void _dump_header(const struct os_area_header *h, const char *func,
 	int line)
 {
+	char str[sizeof(h->magic_num) + 1];
+
+	dump_field(str, h->magic_num, sizeof(h->magic_num));
 	pr_debug("%s:%d: h.magic_num:       '%s'\n", func, line,
-		h->magic_num);
+		str);
 	pr_debug("%s:%d: h.hdr_version:     %u\n", func, line,
 		h->hdr_version);
 	pr_debug("%s:%d: h.db_area_offset:  %u\n", func, line,

@@ -311,7 +326,8 @@ static void _dump_params(const struct os
 
 static int verify_header(const struct os_area_header *header)
 {
-	if (memcmp(header->magic_num, "cell_ext_os_area", 16)) {
+	if (memcmp(header->magic_num, OS_AREA_HEADER_MAGIC_NUM,
+		sizeof(header->magic_num))) {
 		pr_debug("%s:%d magic_num failed\n", __func__, __LINE__);
 		return -1;
 	}

@@ -331,7 +347,8 @@ static int verify_header(const struct os
 
 static int db_verify(const struct os_area_db *db)
 {
-	if (db->magic_num != OS_AREA_DB_MAGIC_NUM) {
+	if (memcmp(db->magic_num, OS_AREA_DB_MAGIC_NUM,
+		sizeof(db->magic_num))) {
 		pr_debug("%s:%d magic_num failed\n", __func__, __LINE__);
 		return -1;
 	}

@@ -484,8 +501,11 @@ static int db_get_rtc_diff(const struct 
 static void _dump_db(const struct os_area_db *db, const char *func,
 	int line)
 {
+	char str[sizeof(db->magic_num) + 1];
+
+	dump_field(str, db->magic_num, sizeof(db->magic_num));
 	pr_debug("%s:%d: db.magic_num:      '%s'\n", func, line,
-		(const char*)&db->magic_num);
+		str);
 	pr_debug("%s:%d: db.version:         %u\n", func, line,
 		db->version);
 	pr_debug("%s:%d: db.index_64:        %u\n", func, line,

@@ -516,7 +536,7 @@ static void os_area_db_init(struct os_ar
 
 	memset(db, 0, sizeof(struct os_area_db));
 
-	db->magic_num = OS_AREA_DB_MAGIC_NUM;
+	memcpy(db->magic_num, OS_AREA_DB_MAGIC_NUM, sizeof(db->magic_num));
 	db->version = 1;
 	db->index_64 = HEADER_SIZE;
 	db->count_64 = VALUES_64_COUNT;