| 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: <6892c9fe760_55f09100d4@dwillia2-xfh.jf.intel.com.notmuch>
Date: Tue, 5 Aug 2025 20:20:30 -0700
From: <dan.j.williams@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>, Dan Williams
<dan.j.williams@...el.com>
CC: <linux-coco@...ts.linux.dev>, <linux-pci@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <bhelgaas@...gle.com>, <aik@....com>,
<lukas@...ner.de>, Samuel Ortiz <sameo@...osinc.com>, Xu Yilun
<yilun.xu@...ux.intel.com>
Subject: Re: [PATCH v4 05/10] samples/devsec: Introduce a PCI device-security
bus + endpoint sample
Jonathan Cameron wrote:
> On Thu, 17 Jul 2025 11:33:53 -0700
> Dan Williams <dan.j.williams@...el.com> wrote:
>
> > Establish just enough emulated PCI infrastructure to register a sample
> > TSM (platform security manager) driver and have it discover an IDE + TEE
> > (link encryption + device-interface security protocol (TDISP)) capable
> > device.
> >
> > Use the existing a CONFIG_PCI_BRIDGE_EMUL to emulate an IDE capable root
> > port, and open code the emulation of an endpoint device via simulated
> > configuration cycle responses.
> >
> > The devsec_tsm driver responds to the PCI core TSM operations as if it
> > successfully exercised the given interface security protocol message.
> >
> > The devsec_bus and devsec_tsm drivers can be loaded in either order to
> > reflect cases like SEV-TIO where the TSM is PCI-device firmware, and
> > cases like TDX Connect where the TSM is a software agent running on the
> > host CPU.
> >
> > Follow-on patches add common code for TSM managed IDE establishment. For
> > now, just successfully complete setup and teardown of the DSM (device
> > security manager) context as a building block for management of TDI
> > (trusted device interface) instances.
> >
> > # modprobe devsec_bus
> > devsec_bus devsec_bus: PCI host bridge to bus 10000:00
> > pci_bus 10000:00: root bus resource [bus 00-01]
> > pci_bus 10000:00: root bus resource [mem 0xf000000000-0xffffffffff 64bit]
> > pci 10000:00:00.0: [8086:7075] type 01 class 0x060400 PCIe Root Port
> > pci 10000:00:00.0: PCI bridge to [bus 00]
> > pci 10000:00:00.0: bridge window [io 0x0000-0x0fff]
> > pci 10000:00:00.0: bridge window [mem 0x00000000-0x000fffff]
> > pci 10000:00:00.0: bridge window [mem 0x00000000-0x000fffff 64bit pref]
> > pci 10000:00:00.0: bridge configuration invalid ([bus 00-00]), reconfiguring
> > pci 10000:01:00.0: [8086:ffff] type 00 class 0x000000 PCIe Endpoint
> > pci 10000:01:00.0: BAR 0 [mem 0xf000000000-0xf0001fffff 64bit pref]
> > pci_doe_abort: pci 10000:01:00.0: DOE: [100] Issuing Abort
> > pci_doe_cache_protocols: pci 10000:01:00.0: DOE: [100] Found protocol 0 vid: 1 prot: 1
> > pci 10000:01:00.0: disabling ASPM on pre-1.1 PCIe device. You can enable it with 'pcie_aspm=force'
> > pci 10000:00:00.0: PCI bridge to [bus 01]
> > pci_bus 10000:01: busn_res: [bus 01] end is updated to 01
> > # modprobe devsec_tsm
> > devsec_tsm_pci_probe: pci 10000:01:00.0: devsec: tsm enabled
> > __pci_tsm_init: pci 10000:01:00.0: TSM: Device security capabilities detected ( ide tee ), TSM attach
> >
> > Cc: Bjorn Helgaas <bhelgaas@...gle.com>
> > Cc: Lukas Wunner <lukas@...ner.de>
> > Cc: Samuel Ortiz <sameo@...osinc.com>
> > Cc: Alexey Kardashevskiy <aik@....com>
> > Cc: Xu Yilun <yilun.xu@...ux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
>
> A fairly superficial review. Too much staring at code today
> to check the emulation was right and have any chance of spotting bugs!
>
> > diff --git a/samples/devsec/bus.c b/samples/devsec/bus.c
> > new file mode 100644
> > index 000000000000..675e185fcf79
> > --- /dev/null
> > +++ b/samples/devsec/bus.c
> > @@ -0,0 +1,708 @@
>
> > +static int alloc_devs(struct devsec *devsec)
> > +{
> > + struct device *dev = devsec->dev;
>
> Similar to below. Maybe use it inline.
ok.
> > + for (int i = 0; i < ARRAY_SIZE(devsec->devsec_devs); i++) {
> > + struct devsec_dev *devsec_dev = devsec_dev_alloc(devsec);
> > + int rc;
> > +
> > + if (IS_ERR(devsec_dev))
> > + return PTR_ERR(devsec_dev);
> > + rc = devm_add_action_or_reset(dev, destroy_devsec_dev,
> > + devsec_dev);
> > + if (rc)
> > + return rc;
> > + devsec->devsec_devs[i] = devsec_dev;
> > + }
> > +
> > + return 0;
> > +}
>
>
> > +static int init_port(struct devsec_port *devsec_port)
> > +{
> > + struct pci_bridge_emul *bridge = &devsec_port->bridge;
> > +
> > + *bridge = (struct pci_bridge_emul) {
> > + .conf = {
> > + .vendor = cpu_to_le16(0x8086),
> > + .device = cpu_to_le16(0x7075),
>
> Emulating something real? If not maybe we should get an ID from another space
> (or reserve this one ;)
I am happy to switch to something else, but no, I do not have time to
chase this through PCI SIG. I do not expect this id to cause conflicts,
but no guarantees.
> > + .class_revision = cpu_to_le32(0x1),
> > + .pref_mem_base = cpu_to_le16(PCI_PREF_RANGE_TYPE_64),
> > + .pref_mem_limit = cpu_to_le16(PCI_PREF_RANGE_TYPE_64),
> > + },
>
>
> > +{
> > + struct device *dev = devsec->dev;
>
> Only used once. I'd move it down there.
ok.
>
> > +
> > + for (int i = 0; i < ARRAY_SIZE(devsec->devsec_ports); i++) {
> > + struct devsec_port *devsec_port = devsec_port_alloc();
> > + int rc;
> > +
> > + if (IS_ERR(devsec_port))
> > + return PTR_ERR(devsec_port);
> > + rc = devm_add_action_or_reset(dev, destroy_port, devsec_port);
> > + if (rc)
> > + return rc;
> > + devsec->devsec_ports[i] = devsec_port;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __init devsec_bus_probe(struct platform_device *pdev)
> > +{
> > + int rc;
> > + struct devsec *devsec;
> > + u64 mmio_size = SZ_64G;
> > + struct devsec_sysdata *sd;
> > + struct pci_host_bridge *hb;
> > + struct device *dev = &pdev->dev;
> > + u64 mmio_start = iomem_resource.end + 1 - SZ_64G;
> > +
> > + hb = devm_pci_alloc_host_bridge(
> > + dev, sizeof(*devsec) - sizeof(struct pci_host_bridge));
>
> I'd move dev up a line.
clang-format disagrees and I prefer just letting a tool do my formatting.
[..]
> > +static int __init devsec_tsm_init(void)
> > +{
> > + __devsec_pci_ops = &devsec_pci_ops;
>
> I'm not immediately grasping why this global is needed.
> You never check if it's set, so why not just move definition of devsec_pci_ops
> early enough that can be directly used everywhere.
Because devsec_pci_ops is used inside the ops it declares. So either
forward declare all those ops, or do this pointer assigment dance. I
opted for the latter as it is smaller.
Powered by blists - more mailing lists