Re: [PATCH 10/18] PCI/CMA: Validate Subject Alternative Name in certificates

On Fri, May 8, 2026 at 1:58 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Valid devices with a DSN of 0 are incorrectly rejected
> - [High] SSVID and SSID fields are incorrectly omitted for PCIe bridges
> - [High] Torn read of device identity prevents reauthentication after runtime mutations
> - [High] Kbuild linkage failure when compiled as a loadable module
> - [Medium] Chaining `snprintf` without length checks risks logic errors and out-of-bounds stack reads
> - [Medium] Strict case-sensitive comparison of the Subject Alternative Name risks rejecting valid certificates
> --

+ Lukas

>
> commit 72435325efe665bb825f47731ca5c0081b7acc14
> Author: Lukas Wunner <[email protected]>
> Subject: PCI/CMA: Validate Subject Alternative Name in certificates
>
> This commit introduces functionality to validate the Subject Alternative Name
> (SAN) in PCIe Leaf Certificates. It parses the SAN using an ASN.1 decoder
> and verifies that the identity encoded in the certificate matches the
> device's identity in Config Space.
>
> > diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> > index 16abfd0e17e19..15512512fce7d 100644
> > --- a/drivers/pci/Makefile
> > +++ b/drivers/pci/Makefile
> [ ... ]
> > -obj-$(CONFIG_PCI_CMA)                += cma.o
> > +obj-$(CONFIG_PCI_CMA)                += cma.o cma.asn1.o
> > +$(obj)/cma.o:                        $(obj)/cma.asn1.h
> > +$(obj)/cma.asn1.o:           $(obj)/cma.asn1.c $(obj)/cma.asn1.h
>
> If CONFIG_PCI_CMA is configured as a loadable module, will Kbuild compile
> two separate modules (cma.ko and cma.asn1.ko)?
>
> Because the ASN.1 compiler does not export the cma_decoder symbol, would
> the cma.ko module fail to link during modpost with an undefined symbol
> error?
>
> Should these objects be bundled into a composite object instead? For
> example:
>
> cma_module-y := cma.o cma.asn1.o
> obj-$(CONFIG_PCI_CMA) += cma_module.o

This seems correct

>
> > diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> > index 998fde6366fb8..ee186f3619409 100644
> > --- a/drivers/pci/cma.c
> > +++ b/drivers/pci/cma.c
> [ ... ]
> > +int pci_cma_note_san(void *context, size_t hdrlen, unsigned char tag,
> > +                  const void *value, size_t vlen)
> > +{
> > +     struct pci_cma_x509_context *ctx = context;
> > +
> > +     /* These aren't the drOIDs we're looking for. */
> > +     if (ctx->last_oid != OID_CMA)
> > +             return 0;
> > +
> > +     if (tag != ASN1_UTF8STR ||
> > +         vlen != ctx->expected_len ||
> > +         memcmp(value, ctx->expected_name, vlen) != 0) {
>
> Does this perform a strict case-sensitive comparison of the Subject
> Alternative Name?
>
> If a device vendor provisions the certificate using uppercase hexadecimal
> letters (a common standard for hardware IDs), could this strict memcmp()
> erroneously reject a valid device?

This is incorrect though, as the case is specified in the PCI spec

>
> > +             pci_err(ctx->pdev, "Leaf certificate of slot %u "
> > +                     "has invalid Subject Alternative Name\n", ctx->slot);
> > +             return -EINVAL;
> > +     }
> > +
> > +     ctx->found = true;
> > +
> > +     return 0;
> > +}
> > +
> > +static unsigned int pci_cma_construct_san(struct pci_dev *pdev, char *name)
> > +{
> > +     unsigned int len;
> > +     u64 serial;
> > +
> > +     len = snprintf(name, CMA_NAME_MAX,
> > +                    "Vendor=%04hx:Device=%04hx:CC=%06x:REV=%02hhx",
> > +                    pdev->vendor, pdev->device, pdev->class, pdev->revision);
>
> The commit message mentions handling devices that mutate their identity
> at runtime, but this string is constructed using cached software state
> from probe time (like pdev->vendor).

This I'm not clear on. Do we need to call pci_bus_read_dev_vendor_id()
to get updated values?

>
> Since pci_get_dsn() reads the live hardware state, could a mutated device
> result in an inconsistent string containing a stale vendor/device ID but
> a new DSN? Will reauthentication fail without a full PCI rescan?
>
> Additionally, snprintf() returns the number of characters that would have
> been written, not the actual bytes written.

This is true

>
> If the inputs ever exceed the 79-byte CMA_NAME_MAX buffer (for instance,
> if a quirk mutates pdev->class to exceed 6 hex characters), the string
> will be truncated but len will reflect the untruncated size.

That's true, but I don't see how the fixed length format string could
exceed the size.

So I don't think this is an actual issue, but...

>
> Could this cause the later memcmp() check in pci_cma_note_san() to read
> out of bounds of the ctx->expected_name stack buffer? Should scnprintf()
> be used here instead to track the actual length written?

scnprintf() should be used instead, just in case.

>
> > +
> > +     if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL)
> > +             len += snprintf(name + len, CMA_NAME_MAX - len,
> > +                             ":SSVID=%04hx:SSID=%04hx",
> > +                             pdev->subsystem_vendor, pdev->subsystem_device);
>
> PCIe Type 1 devices (bridges) can also implement the Subsystem Vendor ID
> capability.
>
> If a Type 1 bridge implements this capability and includes these fields in
> its certificate as required by the specification, will this condition
> unconditionally omit them and cause validation to fail?
>
> > +
> > +     serial = pci_get_dsn(pdev);
> > +     if (serial)
> > +             len += snprintf(name + len, CMA_NAME_MAX - len,
> > +                             ":%016llx", serial);
> > +
> > +     return len;
> > +}
>
> Looking at pci_get_dsn(), it returns 0 both when the DSN capability is
> missing and when the device's legitimate serial number is 0.
>
> If a device implements the capability with a valid DSN of 0, will this
> check evaluate to false, omit the field, and reject a valid device?
>
> Would it be safer to explicitly check for the capability's presence via
> pci_find_ext_capability() instead?

I see your point, but I don't think so.

>
> Also, pci_get_dsn() performs two separate 32-bit reads without a lock:
>
> pci_read_config_dword(dev, pos, &dword);
> dsn = (u64)dword;
> pci_read_config_dword(dev, pos + 4, &dword);
> dsn |= ((u64)dword) << 32;
>
> Could a concurrent mutation of the device identity result in a torn
> 64-bit read during this check?

Really?

Alistair

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10