| 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
| ||
|
Message-ID: <20231228133949.GG2543524@black.fi.intel.com>
Date: Thu, 28 Dec 2023 15:39:49 +0200
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: Esther Shimanovich <eshimanovich@...omium.org>,
Mario Limonciello <mario.limonciello@....com>,
Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, Rajat Jain <rajatja@...gle.com>,
Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [PATCH v4] PCI: Relabel JHL6540 on Lenovo X1 Carbon 7,8
Thanks Lukas for including me.
On Thu, Dec 28, 2023 at 02:25:17PM +0100, Lukas Wunner wrote:
> [adding Mika and Mario to the list of recipients, original patch is here:
> https://lore.kernel.org/all/20231221-thunderbolt-pci-patch-4-v4-1-2e136e57c9bc@chromium.org/
> a lot of folks are on vacation, replies might not be sent before Jan 8;
> full quote without further comments below]
>
> On Thu, Dec 21, 2023 at 03:53:42PM -0500, Esther Shimanovich wrote:
> > On Lenovo X1 Carbon Gen 7/8 devices, when a platform enables a policy to
> > distrust removable PCI devices, the build-in USB-C ports stop working at
> > all.
> > This happens because these X1 Carbon models have a unique feature; a
> > Thunderbolt controller that is discrete from the SoC. The software sees
> > this controller, and incorrectly assumes it is a removable PCI device,
> > even though it is fixed to the computer and is wired to the computer's
> > own USB-C ports.
Yes, the ExternalFacingPort applies to root ports so that includes
everything below that.
> > Relabel all the components of the JHL6540 controller as DEVICE_FIXED,
> > and where applicable, external_facing.
> >
> > Ensure that the security policy to distrust external PCI devices works
> > as intended, and that the device's USB-C ports are able to enumerate
> > even when the policy is enabled.
This has been working just fine so far and as far as I can tell there is
no such "policy" in place in the mainline kernel.
> >
> > Signed-off-by: Esther Shimanovich <eshimanovich@...omium.org>
> > ---
> > Changes in v4:
> > - replaced a dmi check in the rootport quirk with a subsystem vendor and
> > device check.
> > - Link to v3: https://lore.kernel.org/r/20231220-thunderbolt-pci-patch-4-v3-1-056fd1717d06@chromium.org
> >
> > Changes in v3:
> > - removed redundant dmi check, as the subsystem vendor check is
> > sufficient
> > - switched to PCI_VENDOR_ID_LENOVO instead of hex code
> > - Link to v2: https://lore.kernel.org/r/20231219-thunderbolt-pci-patch-4-v2-1-ec2d7af45a9b@chromium.org
> >
> > Changes in v2:
> > - nothing new, v1 was just a test run to see if the ASCII diagram would
> > be rendered properly in mutt and k-9
> > - for folks using gmail, make sure to select "show original" on the top
> > right, as otherwise the diagram will be garbled by the standard
> > non-monospace font
> > - Link to v1: https://lore.kernel.org/r/20231219-thunderbolt-pci-patch-4-v1-1-4e8e3773f0a9@chromium.org
> > ---
> > drivers/pci/quirks.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 112 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index ea476252280a..34e43323ff14 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -3873,6 +3873,118 @@ DECLARE_PCI_FIXUP_SUSPEND_LATE(PCI_VENDOR_ID_INTEL,
> > quirk_apple_poweroff_thunderbolt);
> > #endif
> >
> > +/*
> > + * On most ThinkPad Carbon 7/8s, JHL6540 Thunderbolt 3 bridges are set
> > + * incorrectly as DEVICE_REMOVABLE despite being built into the device.
> > + * This is the side effect of a unique hardware configuration.
There is really nothing "unique" here. It's exactly as specified by
this:
https://learn.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports
and being used in many many system already out there and those have been
working just fine.
> > + *
> > + * Normally, Thunderbolt functionality is integrated to the SoC and
> > + * its root ports.
> > + *
> > + * Most devices:
> > + * root port --> USB-C port
> > + *
> > + * But X1 Carbon Gen 7/8 uses Whiskey Lake and Comet Lake SoC, which
> > + * don't come with Thunderbolt functionality. Therefore, a discrete
> > + * Thunderbolt Host PCI controller was added between the root port and
> > + * the USB-C port.
> > + *
> > + * Thinkpad Carbon 7/8s
> > + * (w/ Whiskey Lake and Comet Lake SoC):
> > + * root port --> JHL6540 --> USB-C port
> > + *
> > + * Because the root port is labeled by FW as "ExternalFacingPort", as
> > + * required by the DMAR ACPI spec, the JHL6540 chip is inaccurately
> > + * labeled as DEVICE_REMOVABLE by the kernel pci driver.
> > + * Therefore, the built-in USB-C ports do not enumerate when policies
> > + * forbidding external pci devices are enforced.
> > + *
> > + * This fix relabels the pci components in the built-in JHL6540 chip as
> > + * DEVICE_FIXED, ensuring that the built-in USB-C ports always enumerate
> > + * properly as intended.
> > + *
> > + * This fix also labels the external facing components of the JHL6540 as
> > + * external_facing, so that the pci attach policy works as intended.
> > + *
> > + * The ASCII diagram below describes the pci layout of the JHL6540 chip.
> > + *
> > + * Root Port
> > + * [8086:02b4] or [8086:9db4]
> > + * |
> > + * JHL6540 Chip
> > + * __________________________________________________
> > + * | Bridge |
> > + * | PCI ID -> [8086:15d3] |
> > + * | DEVFN -> (00) |
> > + * | _________________|__________________ |
> > + * | | | | | |
> > + * | Bridge Bridge Bridge Bridge |
> > + * | [8086:15d3] [8086:15d3] [8086:15d3] [8086:15d3] |
> > + * | (00) (08) (10) (20) |
> > + * | | | | | |
> > + * | NHI | USB Controller | |
> > + * | [8086:15d2] | [8086:15d4] | |
> > + * | (00) | (00) | |
> > + * | |___________| |___________| |
> > + * |____________|________________________|____________|
> > + * | |
> > + * USB-C Port USB-C Port
> > + *
> > + *
> > + * Based on what a JHL6549 pci component's pci id, subsystem device id
> > + * and devfn are, we can infer if it is fixed and if it faces a usb port;
> > + * which would mean it is external facing.
> > + * This quirk uses these values to identify the pci components and set the
> > + * properties accordingly.
> > + */
> > +static void carbon_X1_fixup_relabel_alpine_ridge(struct pci_dev *dev)
> > +{
> > + /* Is this JHL6540 PCI component embedded in a Lenovo device? */
> > + if (dev->subsystem_vendor != PCI_VENDOR_ID_LENOVO)
> > + return;
> > +
> > + /* Is this JHL6540 PCI component embedded in an X1 Carbon Gen 7/8? */
> > + if (dev->subsystem_device != 0x22be && // Gen 8
> > + dev->subsystem_device != 0x2292) { // Gen 7
> > + return;
> > + }
> > +
> > + dev_set_removable(&dev->dev, DEVICE_FIXED);
> > +
> > + /* Not all 0x15d3 components are external facing */
> > + if (dev->device == 0x15d3 &&
> > + dev->devfn != 0x08 &&
> > + dev->devfn != 0x20) {
> > + return;
> > + }
> > +
> > + dev->external_facing = true;
> > +}
> > +
> > +/*
> > + * We also need to relabel the root port as a consequence of changing
> > + * the JHL6540's PCIE hierarchy.
> > + */
> > +static void carbon_X1_fixup_rootport_not_removable(struct pci_dev *dev)
> > +{
> > + /* Is this JHL6540 PCI component embedded in a Lenovo device? */
> > + if (dev->subsystem_vendor != PCI_VENDOR_ID_LENOVO)
> > + return;
> > +
> > + /* Is this JHL6540 PCI component embedded in an X1 Carbon Gen 7/8? */
> > + if (dev->subsystem_device != 0x22be && // Gen 8
> > + dev->subsystem_device != 0x2292) { // Gen 7
> > + return;
> > + }
> > +
> > + dev->external_facing = false;
> > +}
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x15d3, carbon_X1_fixup_relabel_alpine_ridge);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x15d2, carbon_X1_fixup_relabel_alpine_ridge);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x15d4, carbon_X1_fixup_relabel_alpine_ridge);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x02b4, carbon_X1_fixup_rootport_not_removable);
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x9db4, carbon_X1_fixup_rootport_not_removable);
This is not scalable at all. You would need to include lots of systems
here. And there should be no issue at all anyways.
Can you elaborate what the issue is and which mainline kernel you are
using to reproduce this?
Powered by blists - more mailing lists