[patch] PS3: Fix printing of os-area magic numbers

[patch] PS3: Fix printing of os-area magic numbers

[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 <Geert.Uytterhoeven@sonycom.com>
Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---

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(-)

--- a/arch/powerpc/platforms/ps3/os-area.c
+++ b/arch/powerpc/platforms/ps3/os-area.c
@@ -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,

Re: [patch] PS3: Fix printing of os-area magic numbers

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1937 bytes --]

On Sat, 17 Nov 2007, Geoff Levand wrote:
> 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 <Geert.Uytterhoeven@sonycom.com>
> Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
> ---
> 
> 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(-)
> 
> --- a/arch/powerpc/platforms/ps3/os-area.c
> +++ b/arch/powerpc/platforms/ps3/os-area.c
> @@ -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);

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:   Geert.Uytterhoeven@sonycom.com	
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

[patch v2] PS3: Fix printing of os-area magic numbers

[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 <Geert.Uytterhoeven@sonycom.com>
Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---
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(-)

--- a/arch/powerpc/platforms/ps3/os-area.c
+++ b/arch/powerpc/platforms/ps3/os-area.c
@@ -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,

Re: [patch v2] PS3: Fix printing of os-area magic numbers

[Paul Mackerras]

Geoff Levand writes:

> 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.

> +	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.

> +	for(s = (u8*)&db->magic_num, d = str;

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

Paul.

Re: [patch v2] PS3: Fix printing of os-area magic numbers

[Geoff Levand]

On 12/20/2007 08:44 PM, Paul Mackerras wrote:
> Geoff Levand writes:
> 
>> 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.
> 
>> +	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.

OK.

>> +	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 

[patch v3] PS3: Fix printing of os-area magic numbers

[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.
Also unify the handling of header and db magic numbers and make both
of type array of u8.

CC: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---
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(-)

--- a/arch/powerpc/platforms/ps3/os-area.c
+++ b/arch/powerpc/platforms/ps3/os-area.c
@@ -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;

Re: [patch v3] PS3: Fix printing of os-area magic numbers

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1062 bytes --]

On Sat, 22 Dec 2007, Geoff Levand wrote:
> +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:   Geert.Uytterhoeven@sonycom.com	
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

Re: [patch v3] PS3: Fix printing of os-area magic numbers

[Geoff Levand]

On 12/23/2007 01:51 AM, Geert Uytterhoeven wrote:
> On Sat, 22 Dec 2007, Geoff Levand wrote:
>> +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.

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

[patch v4] PS3: Fix printing of os-area magic numbers

[Geoff Levand]

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

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.
Also unify the handling of header and db magic numbers and make both
of type array of u8.

CC: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---
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(-)

--- a/arch/powerpc/platforms/ps3/os-area.c
+++ b/arch/powerpc/platforms/ps3/os-area.c
@@ -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;