在 2024/10/8 16:55, Marc Zyngier 写道:
On Tue, 08 Oct 2024 09:24:29 +0100,
Zheng Zengkai <zhengzengkai@xxxxxxxxxx> 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.
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.
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@xxxxxxxxxx>
---
Changes in v2:
- initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning
v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@xxxxxxxxxx/
---
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 ?
Thanks!
