| Message ID | 20241008082429.33646-1-zhengzengkai@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] ACPI: GTDT: simplify acpi_gtdt_init() implementation | expand |
On Tue, 08 Oct 2024 15:04:52 +0100, Zheng Zengkai <zhengzengkai@huawei.com> wrote: > > > 在 2024/10/8 16:55, Marc Zyngier 写道: > > On Tue, 08 Oct 2024 09:24:29 +0100, > > Zheng Zengkai <zhengzengkai@huawei.com> wrote: > >> According to GTDT Table Structure of ACPI specification, the result of > >> expression '(void *)gtdt + gtdt->platform_timer_offset' will be same > >> with the expression '(void *)table + sizeof(struct acpi_table_gtdt)' > > There is no such language in the spec. It simply says "Offset to the > > Platform Timer Structure[] array from the start of this table". > OK, I mean that in current code, the condition of this check is redundant. That's not my reading if it. Where do you see another validity check that makes this one superfluous? > >> in function acpi_gtdt_init(), so the condition of the "invalid timer > >> data" check will never be true, remove the EINVAL error check branch > >> and change to void return type for acpi_gtdt_init() to simplify the > >> function implementation and error handling by callers. > > And ACPI tables are well known to be always correct, right? > Not always, check is needed, but should be changed. You are not changing it, you are getting rid of it, and I don't see you replacing it with anything else. > >> Besides, after commit c2743a36765d ("clocksource: arm_arch_timer: add > >> GTDT support for memory-mapped timer"), acpi_gtdt_init() currently will > >> not be called with parameter platform_timer_count set to NULL and we > >> can explicitly initialize the integer variable which is used for storing > >> the number of platform timers by caller to zero, so there is no need to > >> do null pointer check for platform_timer_count in acpi_gtdt_init(), > >> remove it to make code a bit more concise. > >> > >> Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com> > >> --- > >> Changes in v2: > >> - initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning > >> > >> v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@huawei.com/ > >> --- > >> drivers/acpi/arm64/gtdt.c | 31 +++++++--------------------- > >> drivers/clocksource/arm_arch_timer.c | 6 ++---- > >> include/linux/acpi.h | 2 +- > >> 3 files changed, 11 insertions(+), 28 deletions(-) > >> > >> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c > >> index c0e77c1c8e09..7fe27c0edde7 100644 > >> --- a/drivers/acpi/arm64/gtdt.c > >> +++ b/drivers/acpi/arm64/gtdt.c > >> @@ -147,45 +147,30 @@ bool __init acpi_gtdt_c3stop(int type) > >> * @table: The pointer to GTDT table. > >> * @platform_timer_count: It points to a integer variable which is used > >> * for storing the number of platform timers. > >> - * This pointer could be NULL, if the caller > >> - * doesn't need this info. > >> - * > >> - * Return: 0 if success, -EINVAL if error. > >> */ > >> -int __init acpi_gtdt_init(struct acpi_table_header *table, > >> +void __init acpi_gtdt_init(struct acpi_table_header *table, > >> int *platform_timer_count) > >> { > >> - void *platform_timer; > >> struct acpi_table_gtdt *gtdt; > >> gtdt = container_of(table, struct acpi_table_gtdt, header); > >> acpi_gtdt_desc.gtdt = gtdt; > >> acpi_gtdt_desc.gtdt_end = (void *)table + table->length; > >> acpi_gtdt_desc.platform_timer = NULL; > >> - if (platform_timer_count) > >> - *platform_timer_count = 0; > >> if (table->revision < 2) { > >> pr_warn("Revision:%d doesn't support Platform Timers.\n", > >> table->revision); > >> - return 0; > >> + return; > >> } > >> if (!gtdt->platform_timer_count) { > >> pr_debug("No Platform Timer.\n"); > >> - return 0; > >> + return; > >> } > >> - platform_timer = (void *)gtdt + gtdt->platform_timer_offset; > >> - if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt)) { > >> - pr_err(FW_BUG "invalid timer data.\n"); > >> - return -EINVAL; > >> - } > >> - acpi_gtdt_desc.platform_timer = platform_timer; > >> - if (platform_timer_count) > >> - *platform_timer_count = gtdt->platform_timer_count; > >> - > >> - return 0; > >> + acpi_gtdt_desc.platform_timer = (void *)gtdt + gtdt->platform_timer_offset; > > And now you are trusting something that potentially points to some > > unexpected location, blindly using it. It is bad enough that the > > current checks are pretty poor (no check against the end of the > > table for the first timer entry), but you are making it worse. > > > > M. > > Can I use the second and third bytes (the length) of platform timer > structure to check against the end of the table ? That's how it is supposed to be done indeed. M.
On Wed, Oct 09, 2024 at 12:33:35PM +0100, Marc Zyngier wrote: > On Tue, 08 Oct 2024 15:04:52 +0100, > Zheng Zengkai <zhengzengkai@huawei.com> wrote: > > > > > > 在 2024/10/8 16:55, Marc Zyngier 写道: > > > On Tue, 08 Oct 2024 09:24:29 +0100, > > > Zheng Zengkai <zhengzengkai@huawei.com> wrote: > > >> According to GTDT Table Structure of ACPI specification, the result of > > >> expression '(void *)gtdt + gtdt->platform_timer_offset' will be same > > >> with the expression '(void *)table + sizeof(struct acpi_table_gtdt)' > > > There is no such language in the spec. It simply says "Offset to the > > > Platform Timer Structure[] array from the start of this table". > > OK, I mean that in current code, the condition of this check is redundant. > > That's not my reading if it. Where do you see another validity check > that makes this one superfluous? > > > >> in function acpi_gtdt_init(), so the condition of the "invalid timer > > >> data" check will never be true, remove the EINVAL error check branch > > >> and change to void return type for acpi_gtdt_init() to simplify the > > >> function implementation and error handling by callers. > > > And ACPI tables are well known to be always correct, right? > > Not always, check is needed, but should be changed. > > You are not changing it, you are getting rid of it, and I don't see > you replacing it with anything else. > > > >> Besides, after commit c2743a36765d ("clocksource: arm_arch_timer: add > > >> GTDT support for memory-mapped timer"), acpi_gtdt_init() currently will > > >> not be called with parameter platform_timer_count set to NULL and we > > >> can explicitly initialize the integer variable which is used for storing > > >> the number of platform timers by caller to zero, so there is no need to > > >> do null pointer check for platform_timer_count in acpi_gtdt_init(), > > >> remove it to make code a bit more concise. > > >> > > >> Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com> > > >> --- > > >> Changes in v2: > > >> - initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning > > >> > > >> v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@huawei.com/ > > >> --- > > >> drivers/acpi/arm64/gtdt.c | 31 +++++++--------------------- > > >> drivers/clocksource/arm_arch_timer.c | 6 ++---- > > >> include/linux/acpi.h | 2 +- > > >> 3 files changed, 11 insertions(+), 28 deletions(-) > > >> > > >> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c > > >> index c0e77c1c8e09..7fe27c0edde7 100644 > > >> --- a/drivers/acpi/arm64/gtdt.c > > >> +++ b/drivers/acpi/arm64/gtdt.c > > >> @@ -147,45 +147,30 @@ bool __init acpi_gtdt_c3stop(int type) > > >> * @table: The pointer to GTDT table. > > >> * @platform_timer_count: It points to a integer variable which is used > > >> * for storing the number of platform timers. > > >> - * This pointer could be NULL, if the caller > > >> - * doesn't need this info. > > >> - * > > >> - * Return: 0 if success, -EINVAL if error. > > >> */ > > >> -int __init acpi_gtdt_init(struct acpi_table_header *table, > > >> +void __init acpi_gtdt_init(struct acpi_table_header *table, > > >> int *platform_timer_count) > > >> { > > >> - void *platform_timer; > > >> struct acpi_table_gtdt *gtdt; > > >> gtdt = container_of(table, struct acpi_table_gtdt, header); > > >> acpi_gtdt_desc.gtdt = gtdt; > > >> acpi_gtdt_desc.gtdt_end = (void *)table + table->length; > > >> acpi_gtdt_desc.platform_timer = NULL; > > >> - if (platform_timer_count) > > >> - *platform_timer_count = 0; > > >> if (table->revision < 2) { > > >> pr_warn("Revision:%d doesn't support Platform Timers.\n", > > >> table->revision); > > >> - return 0; > > >> + return; > > >> } > > >> if (!gtdt->platform_timer_count) { > > >> pr_debug("No Platform Timer.\n"); > > >> - return 0; > > >> + return; > > >> } > > >> - platform_timer = (void *)gtdt + gtdt->platform_timer_offset; > > >> - if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt)) { > > >> - pr_err(FW_BUG "invalid timer data.\n"); > > >> - return -EINVAL; > > >> - } > > >> - acpi_gtdt_desc.platform_timer = platform_timer; > > >> - if (platform_timer_count) > > >> - *platform_timer_count = gtdt->platform_timer_count; > > >> - > > >> - return 0; > > >> + acpi_gtdt_desc.platform_timer = (void *)gtdt + gtdt->platform_timer_offset; > > > And now you are trusting something that potentially points to some > > > unexpected location, blindly using it. It is bad enough that the > > > current checks are pretty poor (no check against the end of the > > > table for the first timer entry), but you are making it worse. > > > > > > M. > > > > Can I use the second and third bytes (the length) of platform timer > > structure to check against the end of the table ? > > That's how it is supposed to be done indeed. AFAICS I think first we need to check whether the platform_timer pointer is within gtdt bounds (< gtdt_end) before de-referencing what it points at to detect the (first) GT entry length and check that it is within gtdt_end too. We do that only in next_platform_timer() for subsequent GT blocks. I agree with Marc, current check is fine, we should add to it, not remove it. Thanks, Lorenzo
在 2024/10/9 19:33, Marc Zyngier 写道: > On Tue, 08 Oct 2024 15:04:52 +0100, > Zheng Zengkai <zhengzengkai@huawei.com> wrote: >> >> 在 2024/10/8 16:55, Marc Zyngier 写道: >>> On Tue, 08 Oct 2024 09:24:29 +0100, >>> Zheng Zengkai <zhengzengkai@huawei.com> wrote: >>>> According to GTDT Table Structure of ACPI specification, the result of >>>> expression '(void *)gtdt + gtdt->platform_timer_offset' will be same >>>> with the expression '(void *)table + sizeof(struct acpi_table_gtdt)' >>> There is no such language in the spec. It simply says "Offset to the >>> Platform Timer Structure[] array from the start of this table". >> OK, I mean that in current code, the condition of this check is redundant. > That's not my reading if it. Where do you see another validity check > that makes this one superfluous? > >>>> in function acpi_gtdt_init(), so the condition of the "invalid timer >>>> data" check will never be true, remove the EINVAL error check branch >>>> and change to void return type for acpi_gtdt_init() to simplify the >>>> function implementation and error handling by callers. >>> And ACPI tables are well known to be always correct, right? >> Not always, check is needed, but should be changed. > You are not changing it, you are getting rid of it, and I don't see > you replacing it with anything else. > >>>> Besides, after commit c2743a36765d ("clocksource: arm_arch_timer: add >>>> GTDT support for memory-mapped timer"), acpi_gtdt_init() currently will >>>> not be called with parameter platform_timer_count set to NULL and we >>>> can explicitly initialize the integer variable which is used for storing >>>> the number of platform timers by caller to zero, so there is no need to >>>> do null pointer check for platform_timer_count in acpi_gtdt_init(), >>>> remove it to make code a bit more concise. >>>> >>>> Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com> >>>> --- >>>> Changes in v2: >>>> - initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning >>>> >>>> v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@huawei.com/ >>>> --- >>>> drivers/acpi/arm64/gtdt.c | 31 +++++++--------------------- >>>> drivers/clocksource/arm_arch_timer.c | 6 ++---- >>>> include/linux/acpi.h | 2 +- >>>> 3 files changed, 11 insertions(+), 28 deletions(-) >>>> >>>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c >>>> index c0e77c1c8e09..7fe27c0edde7 100644 >>>> --- a/drivers/acpi/arm64/gtdt.c >>>> +++ b/drivers/acpi/arm64/gtdt.c >>>> @@ -147,45 +147,30 @@ bool __init acpi_gtdt_c3stop(int type) >>>> * @table: The pointer to GTDT table. >>>> * @platform_timer_count: It points to a integer variable which is used >>>> * for storing the number of platform timers. >>>> - * This pointer could be NULL, if the caller >>>> - * doesn't need this info. >>>> - * >>>> - * Return: 0 if success, -EINVAL if error. >>>> */ >>>> -int __init acpi_gtdt_init(struct acpi_table_header *table, >>>> +void __init acpi_gtdt_init(struct acpi_table_header *table, >>>> int *platform_timer_count) >>>> { >>>> - void *platform_timer; >>>> struct acpi_table_gtdt *gtdt; >>>> gtdt = container_of(table, struct acpi_table_gtdt, header); >>>> acpi_gtdt_desc.gtdt = gtdt; >>>> acpi_gtdt_desc.gtdt_end = (void *)table + table->length; >>>> acpi_gtdt_desc.platform_timer = NULL; >>>> - if (platform_timer_count) >>>> - *platform_timer_count = 0; >>>> if (table->revision < 2) { >>>> pr_warn("Revision:%d doesn't support Platform Timers.\n", >>>> table->revision); >>>> - return 0; >>>> + return; >>>> } >>>> if (!gtdt->platform_timer_count) { >>>> pr_debug("No Platform Timer.\n"); >>>> - return 0; >>>> + return; >>>> } >>>> - platform_timer = (void *)gtdt + gtdt->platform_timer_offset; >>>> - if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt)) { >>>> - pr_err(FW_BUG "invalid timer data.\n"); >>>> - return -EINVAL; >>>> - } >>>> - acpi_gtdt_desc.platform_timer = platform_timer; >>>> - if (platform_timer_count) >>>> - *platform_timer_count = gtdt->platform_timer_count; >>>> - >>>> - return 0; >>>> + acpi_gtdt_desc.platform_timer = (void *)gtdt + gtdt->platform_timer_offset; >>> And now you are trusting something that potentially points to some >>> unexpected location, blindly using it. It is bad enough that the >>> current checks are pretty poor (no check against the end of the >>> table for the first timer entry), but you are making it worse. >>> >>> M. >> Can I use the second and third bytes (the length) of platform timer >> structure to check against the end of the table ? > That's how it is supposed to be done indeed. OK, I will send another patch to add check against the end of the table for the first platform timer entry. Thanks! > M. >
diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c index c0e77c1c8e09..7fe27c0edde7 100644 --- a/drivers/acpi/arm64/gtdt.c +++ b/drivers/acpi/arm64/gtdt.c @@ -147,45 +147,30 @@ bool __init acpi_gtdt_c3stop(int type) * @table: The pointer to GTDT table. * @platform_timer_count: It points to a integer variable which is used * for storing the number of platform timers. - * This pointer could be NULL, if the caller - * doesn't need this info. - * - * Return: 0 if success, -EINVAL if error. */ -int __init acpi_gtdt_init(struct acpi_table_header *table, +void __init acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count) { - void *platform_timer; struct acpi_table_gtdt *gtdt; gtdt = container_of(table, struct acpi_table_gtdt, header); acpi_gtdt_desc.gtdt = gtdt; acpi_gtdt_desc.gtdt_end = (void *)table + table->length; acpi_gtdt_desc.platform_timer = NULL; - if (platform_timer_count) - *platform_timer_count = 0; if (table->revision < 2) { pr_warn("Revision:%d doesn't support Platform Timers.\n", table->revision); - return 0; + return; } if (!gtdt->platform_timer_count) { pr_debug("No Platform Timer.\n"); - return 0; + return; } - platform_timer = (void *)gtdt + gtdt->platform_timer_offset; - if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt)) { - pr_err(FW_BUG "invalid timer data.\n"); - return -EINVAL; - } - acpi_gtdt_desc.platform_timer = platform_timer; - if (platform_timer_count) - *platform_timer_count = gtdt->platform_timer_count; - - return 0; + acpi_gtdt_desc.platform_timer = (void *)gtdt + gtdt->platform_timer_offset; + *platform_timer_count = gtdt->platform_timer_count; } static int __init gtdt_parse_timer_block(struct acpi_gtdt_timer_block *block, @@ -377,7 +362,7 @@ static int __init gtdt_sbsa_gwdt_init(void) { void *platform_timer; struct acpi_table_header *table; - int ret, timer_count, gwdt_count = 0; + int ret = 0, timer_count = 0, gwdt_count = 0; if (acpi_disabled) return 0; @@ -394,8 +379,8 @@ static int __init gtdt_sbsa_gwdt_init(void) * to re-initialize them with permanent mapped pointer values to let the * GTDT parsing possible. */ - ret = acpi_gtdt_init(table, &timer_count); - if (ret || !timer_count) + acpi_gtdt_init(table, &timer_count); + if (!timer_count) goto out_put_gtdt; for_each_platform_timer(platform_timer) { diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 03733101e231..4ca06aba68a4 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -1741,7 +1741,7 @@ static int __init arch_timer_mem_acpi_init(int platform_timer_count) /* Initialize per-processor generic timer and memory-mapped timer(if present) */ static int __init arch_timer_acpi_init(struct acpi_table_header *table) { - int ret, platform_timer_count; + int ret, platform_timer_count = 0; if (arch_timers_present & ARCH_TIMER_TYPE_CP15) { pr_warn("already initialized, skipping\n"); @@ -1750,9 +1750,7 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table) arch_timers_present |= ARCH_TIMER_TYPE_CP15; - ret = acpi_gtdt_init(table, &platform_timer_count); - if (ret) - return ret; + acpi_gtdt_init(table, &platform_timer_count); arch_timer_ppi[ARCH_TIMER_PHYS_NONSECURE_PPI] = acpi_gtdt_map_ppi(ARCH_TIMER_PHYS_NONSECURE_PPI); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 4d5ee84c468b..2e2b168f3790 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -750,7 +750,7 @@ int acpi_reconfig_notifier_register(struct notifier_block *nb); int acpi_reconfig_notifier_unregister(struct notifier_block *nb); #ifdef CONFIG_ACPI_GTDT -int acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count); +void __init acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count); int acpi_gtdt_map_ppi(int type); bool acpi_gtdt_c3stop(int type); int acpi_arch_timer_mem_init(struct arch_timer_mem *timer_mem, int *timer_count);
According to GTDT Table Structure of ACPI specification, the result of expression '(void *)gtdt + gtdt->platform_timer_offset' will be same with the expression '(void *)table + sizeof(struct acpi_table_gtdt)' in function acpi_gtdt_init(), so the condition of the "invalid timer data" check will never be true, remove the EINVAL error check branch and change to void return type for acpi_gtdt_init() to simplify the function implementation and error handling by callers. Besides, after commit c2743a36765d ("clocksource: arm_arch_timer: add GTDT support for memory-mapped timer"), acpi_gtdt_init() currently will not be called with parameter platform_timer_count set to NULL and we can explicitly initialize the integer variable which is used for storing the number of platform timers by caller to zero, so there is no need to do null pointer check for platform_timer_count in acpi_gtdt_init(), remove it to make code a bit more concise. Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com> --- Changes in v2: - initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@huawei.com/ --- drivers/acpi/arm64/gtdt.c | 31 +++++++--------------------- drivers/clocksource/arm_arch_timer.c | 6 ++---- include/linux/acpi.h | 2 +- 3 files changed, 11 insertions(+), 28 deletions(-)