Re: [PATCH 3/4] acpi: fix OEM ID/OEM Table ID padding

[Igor Mammedov]

On Mon, 31 Jan 2022 19:51:24 +0530 (IST)
Ani Sinha <ani@anisinha.ca> wrote:

> On Mon, 31 Jan 2022, Igor Mammedov wrote:
> 
> > On Mon, 31 Jan 2022 18:58:57 +0530 (IST)
> > Ani Sinha <ani@anisinha.ca> wrote:
> >  
> > > On Mon, 31 Jan 2022, Igor Mammedov wrote:
> > >  
> > > > On Mon, 31 Jan 2022 11:47:00 +0530
> > > > Ani Sinha <ani@anisinha.ca> wrote:
> > > >  
> > > > > On Wed, Jan 12, 2022 at 6:33 PM Igor Mammedov <imammedo@redhat.com> 
> > > > > wrote:  
> > > > > >
> > > > > > Commit [2] broke original '\0' padding of OEM ID and OEM Table ID
> > > > > > fields in headers of ACPI tables. While it doesn't have impact on
> > > > > > default values since QEMU uses 6 and 8 characters long values
> > > > > > respectively, it broke usecase where IDs are provided on QEMU CLI.
> > > > > > It shouldn't affect guest (but may cause licensing verification
> > > > > > issues in guest OS).
> > > > > > One of the broken usecases is user supplied SLIC table with IDs
> > > > > > shorter than max possible length, where [2] mangles IDs with extra
> > > > > > spaces in RSDT and FADT tables whereas guest OS expects those to
> > > > > > mirror the respective values of the used SLIC table.
> > > > > >
> > > > > > Fix it by replacing whitespace padding with '\0' padding in
> > > > > > accordance with [1] and expectations of guest OS
> > > > > >
> > > > > > 1) ACPI spec, v2.0b
> > > > > >        17.2 AML Grammar Definition
> > > > > >        ...
> > > > > >        //OEM ID of up to 6 characters. If the OEM ID is
> > > > > >        //shorter than 6 characters, it can be terminated
> > > > > >        //with a NULL character.  
> > > > >
> > > > > On the other hand, from
> > > > > https://uefi.org/specs/ACPI/6.4/21_ACPI_Data_Tables_and_Table_Def_Language/ACPI_Data_Tables.html
> > > > > ,
> > > > >
> > > > > "For example, the OEM ID and OEM Table ID in the common ACPI table
> > > > > header (shown above) are fixed at six and eight characters,
> > > > > respectively. They are not necessarily null terminated"
> > > > >
> > > > > I also checked version 5 and the verbiage is the same. I think not
> > > > > terminating with a null is not incorrect.  
> > > >
> > > > I have a trouble with too much 'not' within the sentence.  
> > >
> > > :-)
> > >  
> > > > So what's the point of this comment and how it's related to
> > > > this patch?  
> > >
> > > My understanding of the spec is that null termination of both those IDs is
> > > not mandatory. Guests may get confused or expect the strings to be null
> > > termimated but they should really be open to expecting non-null terminated
> > > strings as well. What is important is that the number of chars of those
> > > two strings are fixed and well defined in the spec and qemu
> > > implementation.
> > >
> > > In any case, I think we can leave the patch as is for now and see if the
> > > change causes trouble with other guests.  
> >
> >
> > these fields have a fixed length so one doesn't need terminating NULL
> > in case the full length of the field is utilized, otherwise in case of
> > where the value is shorter than max length it has to be null terminated
> > to express a shorter value. That way QEMU worked for years until
> > 602b458201 introduced regression.
> >  
> 
> My comment was based on what I interpreted from reading the latest
> version of the specs. I guess the spec does not explicitly say what the
> padding
> bytes would be in case the length of the IDs are less the max length. I
> interpreted the wording to mean that whether or not the
> length of the string is shorter, one should not expect it to terminate with 
> null.

that's what AML grmamar quoted in commit message clarifies
for specific field(s), as opposed to your generic string
type description

> It would be nice if a future version of the spec made is explicit and
> clearer.


PS:
you were asking the other day if there is any bugs left in ACPI,
(the answer is that I'm not aware of any).
But there are issues with SMBIOS tables that need to be fixed
(it's corner cases with large VM configurations), are you
interested in trying to fix it?