On Mon, Feb 28, 2022 at 10:02:43PM +0100, Ard Biesheuvel wrote:
> On Mon, 28 Feb 2022 at 21:47, Andy Shevchenko <[email protected]> wrote:
> >
> > On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <[email protected]> wrote:
> > >
> > > From: Alexander Graf <[email protected]>
> > >
> > > We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> > > entries of the respective devices. However, when making structs for
> > > matching, we squeeze those IDs into acpi_device_id, which only has 9
> > > bytes space to store the identifier. The subsystem actually captures the
> > > full length of the IDs, and the modalias has the full length, but this
> > > struct we use for matching is limited. It originally had 16 bytes, but
> > > was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> > > modalias generation safe for cross compiling"), presumably on the theory
> > > that it would match the ACPI spec so it didn't matter.
> >
> > > Unfortunately, while most people adhere to the ACPI specs, Microsoft
> > > decided that its VM Generation Counter device [1] should only be
> > > identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> > > than 9 characters.
> >
> > Then why do we not see the ECR from somebody to update the spec or to
> > fix MS' abuse of it?
> > I believe _this_ should be the prerequisite to the proposed change.
>
> What exactly are you suggesting here? That the contributor of this
> patch joins the UEFI forum as an individual adopter in order to get
> the ACPI spec updated before we can advance with this patch? Or that
> he works with Microsoft to get them to refrain from violating it?
>
> I don't think that is reasonable or realistic. The kernel is already
> riddled with UEFI and ACPI quirks that are only there because some
> teams at MS don't take the ACPI spec too literally (which is why they
> have their own AML compiler, for one), and PC vendors only care about
> the Windows sticker, so they don't care about the ACPI spec either.
>
> So I don't think this is the right time to get pedantic about this.
> Our ACPI subsystem already deals with CIDs that are longer than 8
> characters (which are btw permitted by the ACPI spec for bus topology
> related metadata), the only thing being changed here is the ability to
> actually match against such identifiers.
My point is that this is clear abuse of the spec and:
1) we have to enable the broken, because it is already in the wild with
the comment that this is an issue
AND
2) issue an ECR / work with MS to make sure they understand the problem.
This can be done in parallel. What I meant as a prerequisite is to start doing
2) while we have 1) on table.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On 28.02.22 22:27, Andy Shevchenko wrote:
> On Mon, Feb 28, 2022 at 10:02:43PM +0100, Ard Biesheuvel wrote:
>> On Mon, 28 Feb 2022 at 21:47, Andy Shevchenko <[email protected]> wrote:
>>> On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <[email protected]> wrote:
>>>> From: Alexander Graf <[email protected]>
>>>>
>>>> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
>>>> entries of the respective devices. However, when making structs for
>>>> matching, we squeeze those IDs into acpi_device_id, which only has 9
>>>> bytes space to store the identifier. The subsystem actually captures the
>>>> full length of the IDs, and the modalias has the full length, but this
>>>> struct we use for matching is limited. It originally had 16 bytes, but
>>>> was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
>>>> modalias generation safe for cross compiling"), presumably on the theory
>>>> that it would match the ACPI spec so it didn't matter.
>>>> Unfortunately, while most people adhere to the ACPI specs, Microsoft
>>>> decided that its VM Generation Counter device [1] should only be
>>>> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
>>>> than 9 characters.
>>> Then why do we not see the ECR from somebody to update the spec or to
>>> fix MS' abuse of it?
>>> I believe _this_ should be the prerequisite to the proposed change.
>> What exactly are you suggesting here? That the contributor of this
>> patch joins the UEFI forum as an individual adopter in order to get
>> the ACPI spec updated before we can advance with this patch? Or that
>> he works with Microsoft to get them to refrain from violating it?
>>
>> I don't think that is reasonable or realistic. The kernel is already
>> riddled with UEFI and ACPI quirks that are only there because some
>> teams at MS don't take the ACPI spec too literally (which is why they
>> have their own AML compiler, for one), and PC vendors only care about
>> the Windows sticker, so they don't care about the ACPI spec either.
>>
>> So I don't think this is the right time to get pedantic about this.
>> Our ACPI subsystem already deals with CIDs that are longer than 8
>> characters (which are btw permitted by the ACPI spec for bus topology
>> related metadata), the only thing being changed here is the ability to
>> actually match against such identifiers.
> My point is that this is clear abuse of the spec and:
> 1) we have to enable the broken, because it is already in the wild with
> the comment that this is an issue
>
> AND
>
> 2) issue an ECR / work with MS to make sure they understand the problem.
>
> This can be done in parallel. What I meant as a prerequisite is to start doing
> 2) while we have 1) on table.
While trying to revalidate whether this really is breaking the spec,
I've tried to reread the respective section in it and I'm afraid that it
may be valid use of the _CID identifier:
"""
6.1.2 _CID (Compatible ID)
This optional object is used to supply OSPM with a device’s Plug and
Play-Compatible Device ID. Use _CID objects when a device has no other
defined hardware standard method to report its compatible IDs. The _CID
object is valid only within a Full Device Descriptor. An _HID object
must also be present.
Arguments:
None
Return Value:
An Integer or String containing a single CID or a Package containing a
list of CIDs A _CID object evaluates to either:
*
A single Compatible Device ID
*
A package of Compatible Device IDs for the device – in the order of
preference, highest preference first.
Each Compatible Device ID must be either:
*
A valid HID value (a 32-bit compressed EISA type ID or a string such
as “ACPI0004”).
*
A string that uses a bus-specific nomenclature. For example, _CID
can be used to specify the PCI ID. The format of a PCI ID string is
one of the following:
"PCI\CC_ccss" "PCI\CC_ccsspp"
"PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss&REV_rr"
"PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss" "PCI\VEN_vvvv&DEV_dddd&REV_rr"
"PCI\VEN_vvvv&DEV_dddd"
"""
In this case, you could interpret things as looking at "bus-specific
nomenclature" case which even in the examples mentioned in the spec
exceeds the 8 character limit we impose on the matching logic today.
There still is spec violation in Hyper-V's VMGenID device's _HID value
which doesn't follow the PNP format, but that's not relevant here. _CID
doesn't seem to have the same restrictions?
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
On Mon, 28 Feb 2022 at 23:01, Alexander Graf <[email protected]> wrote:
>
> Hi Andy,
>
> On 28.02.22 22:27, Andy Shevchenko wrote:
> > On Mon, Feb 28, 2022 at 10:02:43PM +0100, Ard Biesheuvel wrote:
> >> On Mon, 28 Feb 2022 at 21:47, Andy Shevchenko <[email protected]> wrote:
> >>> On Mon, Feb 28, 2022 at 9:28 PM Jason A. Donenfeld <[email protected]> wrote:
> >>>> From: Alexander Graf <[email protected]>
> >>>>
> >>>> We create a list of ACPI "PNP" IDs which contains _HID, _CID, and CLS
> >>>> entries of the respective devices. However, when making structs for
> >>>> matching, we squeeze those IDs into acpi_device_id, which only has 9
> >>>> bytes space to store the identifier. The subsystem actually captures the
> >>>> full length of the IDs, and the modalias has the full length, but this
> >>>> struct we use for matching is limited. It originally had 16 bytes, but
> >>>> was changed to only have 9 in 6543becf26ff ("mod/file2alias: make
> >>>> modalias generation safe for cross compiling"), presumably on the theory
> >>>> that it would match the ACPI spec so it didn't matter.
> >>>> Unfortunately, while most people adhere to the ACPI specs, Microsoft
> >>>> decided that its VM Generation Counter device [1] should only be
> >>>> identifiable by _CID with a value of "VM_Gen_Counter", which is longer
> >>>> than 9 characters.
> >>> Then why do we not see the ECR from somebody to update the spec or to
> >>> fix MS' abuse of it?
> >>> I believe _this_ should be the prerequisite to the proposed change.
> >> What exactly are you suggesting here? That the contributor of this
> >> patch joins the UEFI forum as an individual adopter in order to get
> >> the ACPI spec updated before we can advance with this patch? Or that
> >> he works with Microsoft to get them to refrain from violating it?
> >>
> >> I don't think that is reasonable or realistic. The kernel is already
> >> riddled with UEFI and ACPI quirks that are only there because some
> >> teams at MS don't take the ACPI spec too literally (which is why they
> >> have their own AML compiler, for one), and PC vendors only care about
> >> the Windows sticker, so they don't care about the ACPI spec either.
> >>
> >> So I don't think this is the right time to get pedantic about this.
> >> Our ACPI subsystem already deals with CIDs that are longer than 8
> >> characters (which are btw permitted by the ACPI spec for bus topology
> >> related metadata), the only thing being changed here is the ability to
> >> actually match against such identifiers.
> > My point is that this is clear abuse of the spec and:
> > 1) we have to enable the broken, because it is already in the wild with
> > the comment that this is an issue
> >
> > AND
> >
> > 2) issue an ECR / work with MS to make sure they understand the problem.
> >
> > This can be done in parallel. What I meant as a prerequisite is to start doing
> > 2) while we have 1) on table.
>
>
> While trying to revalidate whether this really is breaking the spec,
> I've tried to reread the respective section in it and I'm afraid that it
> may be valid use of the _CID identifier:
>
>
> """
>
> 6.1.2 _CID (Compatible ID)
>
> This optional object is used to supply OSPM with a device’s Plug and
> Play-Compatible Device ID. Use _CID objects when a device has no other
> defined hardware standard method to report its compatible IDs. The _CID
> object is valid only within a Full Device Descriptor. An _HID object
> must also be present.
>
> Arguments:
>
> None
>
> Return Value:
> An Integer or String containing a single CID or a Package containing a
> list of CIDs A _CID object evaluates to either:
>
> *
>
> A single Compatible Device ID
>
> *
>
> A package of Compatible Device IDs for the device – in the order of
> preference, highest preference first.
>
> Each Compatible Device ID must be either:
>
> *
>
> A valid HID value (a 32-bit compressed EISA type ID or a string such
> as “ACPI0004”).
>
> *
>
> A string that uses a bus-specific nomenclature. For example, _CID
> can be used to specify the PCI ID. The format of a PCI ID string is
> one of the following:
>
> "PCI\CC_ccss" "PCI\CC_ccsspp"
> "PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss&REV_rr"
> "PCI\VEN_vvvv&DEV_dddd&SUBSYS_ssssssss" "PCI\VEN_vvvv&DEV_dddd&REV_rr"
> "PCI\VEN_vvvv&DEV_dddd"
>
> """
>
> In this case, you could interpret things as looking at "bus-specific
> nomenclature" case which even in the examples mentioned in the spec
> exceeds the 8 character limit we impose on the matching logic today.
>
This is what I was referring to when I mentioned bus topology related metadata.
This is why those uses of ACPI_ID_LEN outside of struct acpi_device_id
may potentially be dangerous, given that _CIDs are apparently
effectively unbounded in size. But relying on this to justify the
"VM_GEN_COUNTER" CID is a bit of a stretch, IMO.
> There still is spec violation in Hyper-V's VMGenID device's _HID value
> which doesn't follow the PNP format, but that's not relevant here. _CID
> doesn't seem to have the same restrictions?
>