linux-kernel - Re: [PATCH] ACPI: FPDT: break out of the loop if record length is zero

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CA+E=qVc424VJsqGRR+SZbDmDdtdVmx+Ag6vt_brhZsNv1JTCRw@mail.gmail.com>
Date:   Tue, 26 Sep 2023 18:47:24 -0700
From:   Vasily Khoruzhick <anarsoul@...il.com>
To:     "Zhang, Rui" <rui.zhang@...el.com>
Cc:     "rafael@...nel.org" <rafael@...nel.org>,
        "lenb@...nel.org" <lenb@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] ACPI: FPDT: break out of the loop if record length is zero

On Mon, Sep 25, 2023 at 10:03 PM Zhang, Rui <rui.zhang@...el.com> wrote:
>
> On Mon, 2023-09-25 at 14:40 -0700, Vasily Khoruzhick wrote:
> > Buggy BIOSes may have zero-length records in FPDT, table, as a result
> s/FPDT, table/FPDT table
>
>
> > fpdt_process_subtable() spins in eternal loop.
> >
> > Break out of the loop if record length is zero.
> >
> >
> > Fixes: d1eb86e59be0 ("ACPI: tables: introduce support for FPDT
> > table")
> > Cc: stable@...r.kernel.org
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@...il.com>
>
> Is there a bugzilla or something where such a buggy BIOS is reported?

I'm not aware of a bug filed a kernel bugzilla, however I found
mentions of the issue online:
https://forum.proxmox.com/threads/acpi-fpdt-duplicate-resume-performance-record-found.114530/

> > ---
> >  drivers/acpi/acpi_fpdt.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c
> > index a2056c4c8cb7..53d8f9601a55 100644
> > --- a/drivers/acpi/acpi_fpdt.c
> > +++ b/drivers/acpi/acpi_fpdt.c
> > @@ -194,6 +194,11 @@ 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_info(FW_BUG "Zero-length record
> > found.\n");
> > +                       break;
>
> For cases like this, it implies the FPDT table is far from usable and
> we should stop processing on such platforms.

Here's FPDT dump:

00000000: 4650 4454 4400 0000 018c 414c 4153 4b41  FPDTD.....ALASKA
00000010: 4120 4d20 4920 0000 0920 0701 414d 4920  A M I ... ..AMI
00000020: 1300 0100 0100 1001 0000 0000 30fe 207f  ............0. .
00000030: 0000 0000 0000 1001 0000 0000 54fe 207f  ............T. .
00000040: 0000 0000                                ....

S3PT at 0x7f20fe30:

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

FBPT at 0x7f20fe54:

7F20FE50: xx xx xx xx 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  *................*

It looks like subtables are not usable. S3PT subtable has the first
record with zero len, and FBPT has its signature again instead of the
first record header.

So yeah, I agree that FPDT is not usabled in this case, and it
shouldn't be processed further.

> So, IMO, it is better to
> 1. return an error here rather than break and return 0.
> 2. add the error handling for fpdt_process_subtable() failures.
>
> what do you think?

Agree, I'll implement it in v2.

Regards,
Vasily