On Wed, Sep 27, 2023 at 9:50 PM Vasily Khoruzhick anarsoul@gmail.com 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: stable@vger.kernel.org Signed-off-by: Vasily Khoruzhick anarsoul@gmail.com

---

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!