Buggy BIOSes may have invalid FPDT subtables, e.g. on my hardware:
S3PT subtable:
7F20FE30: 53 33 50 54 24 00 00 00-00 00 00 00 00 00 18 01 *S3PT$...........*
7F20FE40: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 *................*
7F20FE50: 00 00 00 00
Here the first record has zero length.
FBPT subtable:
7F20FE50: 46 42 50 54-3C 00 00 00 46 42 50 54 *....FBPT<...FBPT*
7F20FE60: 02 00 30 02 00 00 00 00-00 00 00 00 00 00 00 00 *..0.............*
7F20FE70: 2A A6 BC 6E 0B 00 00 00-1A 44 41 70 0B 00 00 00 **..n.....DAp....*
7F20FE80: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 *................*
And here FBPT table has FBPT signature repeated instead of the first
record.
Current code will be looping indefinitely due to zero length records, so
break out of the loop if record length is zero.
While we are here, add proper handling for fpdt_process_subtable()
failures.
Fixes: d1eb86e59be0 ("ACPI: tables: introduce support for FPDT table")
Cc: [email protected]
Signed-off-by: Vasily Khoruzhick <[email protected]>
---
v2: return error from fpdt_process_subtable() if zero-length record is
found and handle fpdt_process_subtable() failures
drivers/acpi/acpi_fpdt.c | 42 ++++++++++++++++++++++++++++++++--------
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c
index a2056c4c8cb7..c97c6e3936cc 100644
--- a/drivers/acpi/acpi_fpdt.c
+++ b/drivers/acpi/acpi_fpdt.c
@@ -194,12 +194,19 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
record_header = (void *)subtable_header + offset;
offset += record_header->length;
+ if (!record_header->length) {
+ pr_err(FW_BUG "Zero-length record found.\n");
+ result = -EINVAL;
+ goto err;
+ }
+
switch (record_header->type) {
case RECORD_S3_RESUME:
if (subtable_type != SUBTABLE_S3PT) {
pr_err(FW_BUG "Invalid record %d for subtable %s\n",
record_header->type, signature);
- return -EINVAL;
+ result = -EINVAL;
+ goto err;
}
if (record_resume) {
pr_err("Duplicate resume performance record found.\n");
@@ -208,7 +215,7 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
record_resume = (struct resume_performance_record *)record_header;
result = sysfs_create_group(fpdt_kobj, &resume_attr_group);
if (result)
- return result;
+ goto err;
break;
case RECORD_S3_SUSPEND:
if (subtable_type != SUBTABLE_S3PT) {
@@ -223,13 +230,14 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
record_suspend = (struct suspend_performance_record *)record_header;
result = sysfs_create_group(fpdt_kobj, &suspend_attr_group);
if (result)
- return result;
+ goto err;
break;
case RECORD_BOOT:
if (subtable_type != SUBTABLE_FBPT) {
pr_err(FW_BUG "Invalid %d for subtable %s\n",
record_header->type, signature);
- return -EINVAL;
+ result = -EINVAL;
+ goto err;
}
if (record_boot) {
pr_err("Duplicate boot performance record found.\n");
@@ -238,7 +246,7 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
record_boot = (struct boot_performance_record *)record_header;
result = sysfs_create_group(fpdt_kobj, &boot_attr_group);
if (result)
- return result;
+ goto err;
break;
default:
@@ -247,6 +255,16 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
}
}
return 0;
+
+err:
+ if (record_boot)
+ sysfs_remove_group(fpdt_kobj, &boot_attr_group);
+ if (record_suspend)
+ sysfs_remove_group(fpdt_kobj, &suspend_attr_group);
+ if (record_resume)
+ sysfs_remove_group(fpdt_kobj, &resume_attr_group);
+
+ return result;
}
static int __init acpi_init_fpdt(void)
@@ -255,6 +273,7 @@ static int __init acpi_init_fpdt(void)
struct acpi_table_header *header;
struct fpdt_subtable_entry *subtable;
u32 offset = sizeof(*header);
+ int result;
status = acpi_get_table(ACPI_SIG_FPDT, 0, &header);
@@ -263,8 +282,8 @@ static int __init acpi_init_fpdt(void)
fpdt_kobj = kobject_create_and_add("fpdt", acpi_kobj);
if (!fpdt_kobj) {
- acpi_put_table(header);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_nomem;
}
while (offset < header->length) {
@@ -272,8 +291,10 @@ static int __init acpi_init_fpdt(void)
switch (subtable->type) {
case SUBTABLE_FBPT:
case SUBTABLE_S3PT:
- fpdt_process_subtable(subtable->address,
+ result = fpdt_process_subtable(subtable->address,
subtable->type);
+ if (result)
+ goto err_subtable;
break;
default:
/* Other types are reserved in ACPI 6.4 spec. */
@@ -282,6 +303,11 @@ static int __init acpi_init_fpdt(void)
offset += sizeof(*subtable);
}
return 0;
+err_subtable:
+ kobject_put(fpdt_kobj);
+err_nomem:
+ acpi_put_table(header);
+ return result;
}
fs_initcall(acpi_init_fpdt);
--
2.42.0
On Wed, Sep 27, 2023 at 9:50 PM Vasily Khoruzhick <[email protected]> wrote:
>
> Buggy BIOSes may have invalid FPDT subtables, e.g. on my hardware:
>
> S3PT subtable:
>
> 7F20FE30: 53 33 50 54 24 00 00 00-00 00 00 00 00 00 18 01 *S3PT$...........*
> 7F20FE40: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 *................*
> 7F20FE50: 00 00 00 00
>
> Here the first record has zero length.
>
> FBPT subtable:
>
> 7F20FE50: 46 42 50 54-3C 00 00 00 46 42 50 54 *....FBPT<...FBPT*
> 7F20FE60: 02 00 30 02 00 00 00 00-00 00 00 00 00 00 00 00 *..0.............*
> 7F20FE70: 2A A6 BC 6E 0B 00 00 00-1A 44 41 70 0B 00 00 00 **..n.....DAp....*
> 7F20FE80: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00 *................*
>
> And here FBPT table has FBPT signature repeated instead of the first
> record.
>
> Current code will be looping indefinitely due to zero length records, so
> break out of the loop if record length is zero.
>
> While we are here, add proper handling for fpdt_process_subtable()
> failures.
>
> Fixes: d1eb86e59be0 ("ACPI: tables: introduce support for FPDT table")
> Cc: [email protected]
> Signed-off-by: Vasily Khoruzhick <[email protected]>
> ---
> v2: return error from fpdt_process_subtable() if zero-length record is
> found and handle fpdt_process_subtable() failures
>
> drivers/acpi/acpi_fpdt.c | 42 ++++++++++++++++++++++++++++++++--------
> 1 file changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c
> index a2056c4c8cb7..c97c6e3936cc 100644
> --- a/drivers/acpi/acpi_fpdt.c
> +++ b/drivers/acpi/acpi_fpdt.c
> @@ -194,12 +194,19 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
> record_header = (void *)subtable_header + offset;
> offset += record_header->length;
>
> + if (!record_header->length) {
> + pr_err(FW_BUG "Zero-length record found.\n");
> + result = -EINVAL;
> + goto err;
> + }
> +
> switch (record_header->type) {
> case RECORD_S3_RESUME:
> if (subtable_type != SUBTABLE_S3PT) {
> pr_err(FW_BUG "Invalid record %d for subtable %s\n",
> record_header->type, signature);
> - return -EINVAL;
> + result = -EINVAL;
> + goto err;
> }
> if (record_resume) {
> pr_err("Duplicate resume performance record found.\n");
> @@ -208,7 +215,7 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
> record_resume = (struct resume_performance_record *)record_header;
> result = sysfs_create_group(fpdt_kobj, &resume_attr_group);
> if (result)
> - return result;
> + goto err;
> break;
> case RECORD_S3_SUSPEND:
> if (subtable_type != SUBTABLE_S3PT) {
> @@ -223,13 +230,14 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
> record_suspend = (struct suspend_performance_record *)record_header;
> result = sysfs_create_group(fpdt_kobj, &suspend_attr_group);
> if (result)
> - return result;
> + goto err;
> break;
> case RECORD_BOOT:
> if (subtable_type != SUBTABLE_FBPT) {
> pr_err(FW_BUG "Invalid %d for subtable %s\n",
> record_header->type, signature);
> - return -EINVAL;
> + result = -EINVAL;
> + goto err;
> }
> if (record_boot) {
> pr_err("Duplicate boot performance record found.\n");
> @@ -238,7 +246,7 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
> record_boot = (struct boot_performance_record *)record_header;
> result = sysfs_create_group(fpdt_kobj, &boot_attr_group);
> if (result)
> - return result;
> + goto err;
> break;
>
> default:
> @@ -247,6 +255,16 @@ static int fpdt_process_subtable(u64 address, u32 subtable_type)
> }
> }
> return 0;
> +
> +err:
> + if (record_boot)
> + sysfs_remove_group(fpdt_kobj, &boot_attr_group);
> + if (record_suspend)
> + sysfs_remove_group(fpdt_kobj, &suspend_attr_group);
> + if (record_resume)
> + sysfs_remove_group(fpdt_kobj, &resume_attr_group);
> +
> + return result;
> }
>
> static int __init acpi_init_fpdt(void)
> @@ -255,6 +273,7 @@ static int __init acpi_init_fpdt(void)
> struct acpi_table_header *header;
> struct fpdt_subtable_entry *subtable;
> u32 offset = sizeof(*header);
> + int result;
>
> status = acpi_get_table(ACPI_SIG_FPDT, 0, &header);
>
> @@ -263,8 +282,8 @@ static int __init acpi_init_fpdt(void)
>
> fpdt_kobj = kobject_create_and_add("fpdt", acpi_kobj);
> if (!fpdt_kobj) {
> - acpi_put_table(header);
> - return -ENOMEM;
> + result = -ENOMEM;
> + goto err_nomem;
> }
>
> while (offset < header->length) {
> @@ -272,8 +291,10 @@ static int __init acpi_init_fpdt(void)
> switch (subtable->type) {
> case SUBTABLE_FBPT:
> case SUBTABLE_S3PT:
> - fpdt_process_subtable(subtable->address,
> + result = fpdt_process_subtable(subtable->address,
> subtable->type);
> + if (result)
> + goto err_subtable;
> break;
> default:
> /* Other types are reserved in ACPI 6.4 spec. */
> @@ -282,6 +303,11 @@ static int __init acpi_init_fpdt(void)
> offset += sizeof(*subtable);
> }
> return 0;
> +err_subtable:
> + kobject_put(fpdt_kobj);
> +err_nomem:
> + acpi_put_table(header);
> + return result;
> }
>
> fs_initcall(acpi_init_fpdt);
> --
Applied (with some minor tweaks) as 6.7 material, thanks!