On Wed, Sep 27, 2023 at 9:50 PM Vasily Khoruzhick <anarsoul@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Vasily Khoruzhick <anarsoul@xxxxxxxxx>
> ---
> 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!
