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(-)
diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
index 864dd46..28a5caf 100644
--- a/drivers/acpi/apei/erst.c
+++ b/drivers/acpi/apei/erst.c
@@ -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;
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index c637b75..821f8ac 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -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 {
--
1.7.1.1
On Tue, 2010-08-17 at 08:54 +0800, Jin Dongming wrote:
> 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?
> 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(-)
>
> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
> index 864dd46..28a5caf 100644
> --- a/drivers/acpi/apei/erst.c
> +++ b/drivers/acpi/apei/erst.c
> @@ -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;
>
> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
> index c637b75..821f8ac 100644
> --- a/include/acpi/actbl1.h
> +++ b/include/acpi/actbl1.h
> @@ -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 */
> +};
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
(2010/08/17 10:33), Huang Ying wrote:
> On Tue, 2010-08-17 at 08:54 +0800, Jin Dongming wrote:
>> 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.
>
>> 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(-)
>>
>> diff --git a/drivers/acpi/apei/erst.c b/drivers/acpi/apei/erst.c
>> index 864dd46..28a5caf 100644
>> --- a/drivers/acpi/apei/erst.c
>> +++ b/drivers/acpi/apei/erst.c
>> @@ -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;
>>
>> diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
>> index c637b75..821f8ac 100644
>> --- a/include/acpi/actbl1.h
>> +++ b/include/acpi/actbl1.h
>> @@ -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 */
>> +};
>
> 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.
>
> Best Regards,
> Huang Ying
>
>
>
>
On Tue, 2010-08-17 at 10:14 +0800, Jin Dongming wrote:
> > 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
(2010/08/17 12:22), Huang Ying wrote:
> On Tue, 2010-08-17 at 10:14 +0800, Jin Dongming wrote:
>>> 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.
>
Yes. It looks good. And I am sorry I am unaware of Yinghai' patch.
> Best Regards,
> Huang Ying
>
>
Best Regards,
Jin Dongming
> --
> 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/
>
>
On Tue, 2010-08-17 at 12:44 +0800, Jin Dongming wrote:
> (2010/08/17 12:22), Huang Ying wrote:
> > On Tue, 2010-08-17 at 10:14 +0800, Jin Dongming wrote:
> >>> 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.
> >
> 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
(2010/08/18 14:34), Huang Ying wrote:
> On Tue, 2010-08-17 at 12:44 +0800, Jin Dongming wrote:
>> (2010/08/17 12:22), Huang Ying wrote:
>>> On Tue, 2010-08-17 at 10:14 +0800, Jin Dongming wrote:
>>>>> 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.
>>>
>> 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.
> Hi, Yinghai,
>
> Do you mind for Dongming to push this fix?
>
> Best REgards,
> Huang Ying
Best Regards,
Jin Dongming
>
>
> --
> 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/
>
>
On 08/17/2010 10:34 PM, Huang Ying wrote:
> On Tue, 2010-08-17 at 12:44 +0800, Jin Dongming wrote:
>> (2010/08/17 12:22), Huang Ying wrote:
>>> On Tue, 2010-08-17 at 10:14 +0800, Jin Dongming wrote:
>>>>> 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.
>>>
>> 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(-)
Index: linux-2.6/drivers/acpi/apei/einj.c
===================================================================
--- linux-2.6.orig/drivers/acpi/apei/einj.c
+++ 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;
Index: linux-2.6/drivers/acpi/apei/erst.c
===================================================================
--- linux-2.6.orig/drivers/acpi/apei/erst.c
+++ 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;
On Sat, 2010-08-21 at 03:26 +0800, Yinghai Lu wrote:
> On 08/17/2010 10:34 PM, Huang Ying wrote:
> > On Tue, 2010-08-17 at 12:44 +0800, Jin Dongming wrote:
> >> (2010/08/17 12:22), Huang Ying wrote:
> >>> On Tue, 2010-08-17 at 10:14 +0800, Jin Dongming wrote:
> >>>>> 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.
> >>>
> >> 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]>
Acked-by: Huang Ying <[email protected]>