Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20230927195035.2174949-1-anarsoul@gmail.com>
In-Reply-To: <20230927195035.2174949-1-anarsoul@gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 3 Oct 2023 21:14:16 +0200
Message-ID: <CAJZ5v0gUOt3v+rsfD0D8JrHCTR_5qrp7PZxCF=0_ZVFVpg6XMQ@mail.gmail.com>
Subject: Re: [PATCH v2] ACPI: FPDT: properly handle invalid FPDT subtables
To:     Vasily Khoruzhick <anarsoul@gmail.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>, Zhang Rui <rui.zhang@intel.com>,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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!