ACPI, APEI, ERST Fix the wrong checking of Serialization Header's length

PATCH In review Jin Dongming 3 participants 9 messages Aug 17, 2010 – Aug 23, 2010

Subsystems (4)

ACPI ACPI component architecture (acpica) ACPI APEI Pstore filesystem

lore

Share & tools

lore

Jin Dongming

author

2010-08-17 00:53 UTC #0 lore raw

PATCH 1/4 ACPI, APEI, ERST Fix the wrong checking of Serialization Header's length

header_length in struct acpi_table_erst is not the length of struct acpi_table_erst, but the length of Serialization Header.

In erst_check_table(), header_length is used for checking the length of struct acpi_table_erst. So I think it is wrong.

This patch fixed it and I confirmed it on x86_64 next-tree.

Signed-off-by Jin Dongming <[email protected]>

---
 drivers/acpi/apei/erst.c |   13 ++++++++-----
 include/acpi/actbl1.h    |   14 +++++++++++---
 2 files changed, 19 insertions(+), 8 deletions(-)

drivers/acpi/apei/erst.c +8 -5

index 864dd46..28a5caf 100644
@@ -358,7 +358,8 @@ static struct apei_exec_ins_type erst_ins_type[] = {
 static inline void erst_exec_ctx_init(struct apei_exec_context *ctx)
 {
 	apei_exec_ctx_init(ctx, erst_ins_type, ARRAY_SIZE(erst_ins_type),
-			   ERST_TAB_ENTRY(erst_tab), erst_tab->entries);
+			   ERST_TAB_ENTRY(erst_tab),
+			   erst_tab->seri_header.entries);
 }
 
 static int erst_get_erange(struct erst_erange *range)

@@ -749,12 +750,14 @@ __setup("erst_disable", setup_erst_disable);
 
 static int erst_check_table(struct acpi_table_erst *erst_tab)
 {
-	if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+	if (erst_tab->common_header.length < sizeof(struct acpi_table_erst))
 		return -EINVAL;
-	if (erst_tab->header.length < sizeof(struct acpi_table_erst))
+
+	if (erst_tab->seri_header.length != sizeof(struct acpi_erst_header))
 		return -EINVAL;
-	if (erst_tab->entries !=
-	    (erst_tab->header.length - sizeof(struct acpi_table_erst)) /
+
+	if (erst_tab->seri_header.entries !=
+	    (erst_tab->common_header.length - sizeof(struct acpi_table_erst)) /
 	    sizeof(struct acpi_erst_entry))
 		return -EINVAL;

include/acpi/actbl1.h +11 -3

index c637b75..821f8ac 100644
@@ -283,13 +283,21 @@ enum acpi_einj_command_status {
  *
  ******************************************************************************/
 
-struct acpi_table_erst {
-	struct acpi_table_header header;	/* Common ACPI table header */
-	u32 header_length;
+/* ERST Serialization Header */
+
+struct acpi_erst_header {
+	u32 length;
 	u32 reserved;
 	u32 entries;
 };
 
+/* ERST Header */
+
+struct acpi_table_erst {
+	struct acpi_table_header common_header;	/* Common ACPI table header */
+	struct acpi_erst_header seri_header;	/* ERST Serialization Header */
+};
+
 /* ERST Serialization Entries (actions) */
 
 struct acpi_erst_entry {

Signature · 1 line

1.7.1.1

Huang Ying

2010-08-17 01:34 UTC #1 lore raw

Quoted 5 lines · Jin Dongming

header_length in struct acpi_table_erst is not the length of struct acpi_table_erst, but the length of Serialization Header.
In erst_check_table(), header_length is used for checking the length of struct acpi_table_erst. So I think it is wrong.

Why "think"? Do you have a machine with header_length set as you said?

Quoted 56 lines

This patch fixed it and I confirmed it on x86_64 next-tree.

Signed-off-by Jin Dongming <[email protected]>

---
 drivers/acpi/apei/erst.c |   13 ++++++++-----
 include/acpi/actbl1.h    |   14 +++++++++++---
 2 files changed, 19 insertions(+), 8 deletions(-)

drivers/acpi/apei/erst.c +8 -5

index 864dd46..28a5caf 100644
@@ -358,7 +358,8 @@ static struct apei_exec_ins_type erst_ins_type[] = {
 static inline void erst_exec_ctx_init(struct apei_exec_context *ctx)
 {
 	apei_exec_ctx_init(ctx, erst_ins_type, ARRAY_SIZE(erst_ins_type),
-			   ERST_TAB_ENTRY(erst_tab), erst_tab->entries);
+			   ERST_TAB_ENTRY(erst_tab),
+			   erst_tab->seri_header.entries);
 }
 
 static int erst_get_erange(struct erst_erange *range)

@@ -749,12 +750,14 @@ __setup("erst_disable", setup_erst_disable);
 
 static int erst_check_table(struct acpi_table_erst *erst_tab)
 {
-	if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+	if (erst_tab->common_header.length < sizeof(struct acpi_table_erst))
 		return -EINVAL;
-	if (erst_tab->header.length < sizeof(struct acpi_table_erst))
+
+	if (erst_tab->seri_header.length != sizeof(struct acpi_erst_header))
 		return -EINVAL;
-	if (erst_tab->entries !=
-	    (erst_tab->header.length - sizeof(struct acpi_table_erst)) /
+
+	if (erst_tab->seri_header.entries !=
+	    (erst_tab->common_header.length - sizeof(struct acpi_table_erst)) /
 	    sizeof(struct acpi_erst_entry))
 		return -EINVAL;

include/acpi/actbl1.h +4 -3

index c637b75..821f8ac 100644
@@ -283,13 +283,21 @@ enum acpi_einj_command_status {
  *
  ******************************************************************************/
 
-struct acpi_table_erst {
-	struct acpi_table_header header;	/* Common ACPI table header */
-	u32 header_length;
+/* ERST Serialization Header */
+
+struct acpi_erst_header {
+	u32 length;

include/acpi/actbl1.h enum acpi_einj_command_status { · line 299

Quoted 10 lines

include/acpi/actbl1.h +6 -0

 	u32 reserved;
 	u32 entries;
 };
 
+/* ERST Header */
+
+struct acpi_table_erst {
+	struct acpi_table_header common_header;	/* Common ACPI table header */
+	struct acpi_erst_header seri_header;	/* ERST Serialization Header */
+};

I don't think it is necessary to change the header definition, and seri_header is not a good name for me.

Best Regards,
Huang Ying

Jin Dongming

author

2010-08-17 02:14 UTC #2 lore raw

Quoted 8 lines · Huang Ying

Quoted 5 lines · Jin Dongming
header_length in struct acpi_table_erst is not the length of struct acpi_table_erst, but the length of Serialization Header.
In erst_check_table(), header_length is used for checking the length of struct acpi_table_erst. So I think it is wrong.
Why "think"? Do you have a machine with header_length set as you said?

I have such machine, and I confirmed that the header_length returned by firmware is same as what I described in the above description.

And further more, according to the spec document of ACPI 4.0(page 515), it is written like following.

  Field                      Byte length   Byte offset   Description
  Serialization Header Size  4             0x24          Length in bytes of

the serialization header.

Quoted 61 lines

This patch fixed it and I confirmed it on x86_64 next-tree.

Signed-off-by Jin Dongming <[email protected]>

---
 drivers/acpi/apei/erst.c |   13 ++++++++-----
 include/acpi/actbl1.h    |   14 +++++++++++---
 2 files changed, 19 insertions(+), 8 deletions(-)

drivers/acpi/apei/erst.c +8 -5

index 864dd46..28a5caf 100644
@@ -358,7 +358,8 @@ static struct apei_exec_ins_type erst_ins_type[] = {
 static inline void erst_exec_ctx_init(struct apei_exec_context *ctx)
 {
 	apei_exec_ctx_init(ctx, erst_ins_type, ARRAY_SIZE(erst_ins_type),
-			   ERST_TAB_ENTRY(erst_tab), erst_tab->entries);
+			   ERST_TAB_ENTRY(erst_tab),
+			   erst_tab->seri_header.entries);
 }
 
 static int erst_get_erange(struct erst_erange *range)

@@ -749,12 +750,14 @@ __setup("erst_disable", setup_erst_disable);
 
 static int erst_check_table(struct acpi_table_erst *erst_tab)
 {
-	if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+	if (erst_tab->common_header.length < sizeof(struct acpi_table_erst))
 		return -EINVAL;
-	if (erst_tab->header.length < sizeof(struct acpi_table_erst))
+
+	if (erst_tab->seri_header.length != sizeof(struct acpi_erst_header))
 		return -EINVAL;
-	if (erst_tab->entries !=
-	    (erst_tab->header.length - sizeof(struct acpi_table_erst)) /
+
+	if (erst_tab->seri_header.entries !=
+	    (erst_tab->common_header.length - sizeof(struct acpi_table_erst)) /
 	    sizeof(struct acpi_erst_entry))
 		return -EINVAL;

include/acpi/actbl1.h +4 -3

index c637b75..821f8ac 100644
@@ -283,13 +283,21 @@ enum acpi_einj_command_status {
  *
  ******************************************************************************/
 
-struct acpi_table_erst {
-	struct acpi_table_header header;	/* Common ACPI table header */
-	u32 header_length;
+/* ERST Serialization Header */
+
+struct acpi_erst_header {
+	u32 length;
 	u32 reserved;
 	u32 entries;
 };

Quoted 9 lines

include/acpi/actbl1.h enum acpi_einj_command_status { · line 299
Quoted 6 lines
include/acpi/actbl1.h +6 -0
+/* ERST Header */
+
+struct acpi_table_erst {
+	struct acpi_table_header common_header;	/* Common ACPI table header */
+	struct acpi_erst_header seri_header;	/* ERST Serialization Header */
+};
I don't think it is necessary to change the header definition, and seri_header is not a good name for me.

Why I added this structure here is that I don't want to do following checking. For example,

	if (header_length != 0x0c) {
		...
	}

Though it is simple, it is really difficult for us to understand the real meaning of the code.

Quoted 7 lines

Best Regards,
Huang Ying

Huang Ying

2010-08-17 03:22 UTC #3 lore raw

Quoted 7 lines · Jin Dongming

Quoted 2 lines
I don't think it is necessary to change the header definition, and seri_header is not a good name for me.
Why I added this structure here is that I don't want to do following checking. For example,
	if (header_length != 0x0c) {
		...
	}

This can be:

static int erst_check_table(struct acpi_table_erst *erst_tab) {
- if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+ if (erst_tab->header_length !=
+ (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))

like Yinghai has been done.

Best Regards,
Huang Ying

Jin Dongming

author

2010-08-17 04:44 UTC #4 lore raw

Quoted 17 lines · Huang Ying

Quoted 7 lines · Jin Dongming
Quoted 2 lines
I don't think it is necessary to change the header definition, and seri_header is not a good name for me.
Why I added this structure here is that I don't want to do following checking. For example,
	if (header_length != 0x0c) {
		...
	}
This can be:
static int erst_check_table(struct acpi_table_erst *erst_tab) {
(quoted diff) +2 -1
-       if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+       if (erst_tab->header_length !=
+           (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))

Quoted 2 lines

like Yinghai has been done.

Yes. It looks good. And I am sorry I am unaware of Yinghai' patch.

Quoted 4 lines

Best Regards,
Huang Ying

Best Regards,
Jin Dongming

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Huang Ying

2010-08-18 05:35 UTC #5 lore raw

Quoted 18 lines · Jin Dongming

Quoted 17 lines · Huang Ying
Quoted 7 lines · Jin Dongming
Quoted 2 lines
I don't think it is necessary to change the header definition, and seri_header is not a good name for me.
Why I added this structure here is that I don't want to do following checking. For example,
	if (header_length != 0x0c) {
		...
	}
This can be:
static int erst_check_table(struct acpi_table_erst *erst_tab) {
(quoted diff) +2 -1
-       if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+       if (erst_tab->header_length !=
+           (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))

Quoted 3 lines

Quoted 2 lines
like Yinghai has been done.
Yes. It looks good. And I am sorry I am unaware of Yinghai' patch.

Maybe you can help push this fix in, it seems that Yinghai is busy on something else?

Hi, Yinghai,

Do you mind for Dongming to push this fix?

Best REgards,
Huang Ying

Jin Dongming

author

2010-08-18 08:35 UTC #6 lore raw

Quoted 19 lines · Huang Ying

Quoted 18 lines · Jin Dongming
Quoted 17 lines · Huang Ying
Quoted 7 lines · Jin Dongming
Quoted 2 lines
I don't think it is necessary to change the header definition, and seri_header is not a good name for me.
Why I added this structure here is that I don't want to do following checking. For example,
	if (header_length != 0x0c) {
		...
	}
This can be:
static int erst_check_table(struct acpi_table_erst *erst_tab) {
(quoted diff) +2 -1
-       if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+       if (erst_tab->header_length !=
+           (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))

Quoted 7 lines

Quoted 3 lines
Quoted 2 lines
like Yinghai has been done.
Yes. It looks good. And I am sorry I am unaware of Yinghai' patch.
Maybe you can help push this fix in, it seems that Yinghai is busy on something else?

It is my pleasure if Yinghai agree with it.

Quoted 6 lines

Hi, Yinghai,
Do you mind for Dongming to push this fix?
Best REgards,
Huang Ying

Best Regards,
Jin Dongming

Quoted 9 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Yinghai Lu

2010-08-20 19:27 UTC #7 lore raw

In reply to Huang Ying

Quoted 21 lines · Huang Ying

Quoted 20 lines · Jin Dongming
Quoted 19 lines · Huang Ying
Quoted 7 lines · Jin Dongming
Quoted 2 lines
I don't think it is necessary to change the header definition, and seri_header is not a good name for me.
Why I added this structure here is that I don't want to do following checking. For example,
	if (header_length != 0x0c) {
		...
	}
This can be:
static int erst_check_table(struct acpi_table_erst *erst_tab) {
(quoted diff) +2 -1
-       if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+       if (erst_tab->header_length !=
+           (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))
like Yinghai has been done.

Quoted 9 lines

Quoted 1 line
Yes. It looks good. And I am sorry I am unaware of Yinghai' patch.
Maybe you can help push this fix in, it seems that Yinghai is busy on something else?
Hi, Yinghai,
Do you mind for Dongming to push this fix?

do you mean this one?

[PATCH] acpi: fix apei related table size checking

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/acpi/apei/einj.c |    3 ++-
 drivers/acpi/apei/erst.c |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

drivers/acpi/apei/einj.c +2 -1

Index: linux-2.6/drivers/acpi/apei/einj.c
===================================================================
@@ -426,7 +426,8 @@ DEFINE_SIMPLE_ATTRIBUTE(error_inject_fop
 
 static int einj_check_table(struct acpi_table_einj *einj_tab)
 {
-	if (einj_tab->header_length != sizeof(struct acpi_table_einj))
+	if (einj_tab->header_length !=
+	    (sizeof(struct acpi_table_einj) - sizeof(einj_tab->header)))
 		return -EINVAL;
 	if (einj_tab->header.length < sizeof(struct acpi_table_einj))
 		return -EINVAL;

drivers/acpi/apei/erst.c +2 -1

Index: linux-2.6/drivers/acpi/apei/erst.c
===================================================================
@@ -750,7 +750,8 @@ __setup("erst_disable", setup_erst_disab
 
 static int erst_check_table(struct acpi_table_erst *erst_tab)
 {
-	if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+	if (erst_tab->header_length !=
+	    (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))
 		return -EINVAL;
 	if (erst_tab->header.length < sizeof(struct acpi_table_erst))
 		return -EINVAL;

Huang Ying

Acked-by

2010-08-23 00:26 UTC #8 lore raw

Quoted 29 lines · Yinghai Lu

Quoted 28 lines · Huang Ying
Quoted 18 lines · Jin Dongming
Quoted 17 lines · Huang Ying
Quoted 7 lines · Jin Dongming
Quoted 2 lines
I don't think it is necessary to change the header definition, and seri_header is not a good name for me.
Why I added this structure here is that I don't want to do following checking. For example,
	if (header_length != 0x0c) {
		...
	}
This can be:
static int erst_check_table(struct acpi_table_erst *erst_tab) {
drivers/acpi/apei/erst.c +2 -1
-       if (erst_tab->header_length != sizeof(struct acpi_table_erst))
+       if (erst_tab->header_length !=
+           (sizeof(struct acpi_table_erst) - sizeof(erst_tab->header)))
Quoted 3 lines
Quoted 2 lines
like Yinghai has been done.
Yes. It looks good. And I am sorry I am unaware of Yinghai' patch.
Maybe you can help push this fix in, it seems that Yinghai is busy on something else?
Hi, Yinghai,

Quoted 7 lines

Quoted 2 lines
Do you mind for Dongming to push this fix?
do you mean this one?
[PATCH] acpi: fix apei related table size checking
Signed-off-by Yinghai Lu <[email protected]>

Acked-by Huang Ying <[email protected]>