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

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

[Alistair Francis]

On Fri, May 8, 2026 at 1:58 PM <sashiko-bot@kernel.org> 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 <lukas@wunner.de>
> 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/20260508031710.514574-1-alistair.francis@wdc.com?part=10

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

[Lukas Wunner]

On Thu, May 14, 2026 at 12:45:46PM +1000, Alistair Francis wrote:
> On Fri, May 8, 2026 at 1:58 PM <sashiko-bot@kernel.org> wrote:
> > > +++ 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

Hallucination alert:

CONFIG_PCI_CMA is bool, so cannot be configured as a loadable module.

> > > +     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

Right.  Another hallucination.

> > > +     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?

If the device mutates its device identity in Config Space at runtime,
e.g. as a result of an FPGA update or hot replacement, it is necessary
to re-enumerate the device either through the standard hotplug
mechanism or manually via sysfs.  Otherwise authentication simply fails
because of the mismatch between (cached) Config Space and Subject
Alternative Name.

I think this is the correct thing to do.  The alternative would be that
we'd just carry on using the device normally even though the mismatch
tells us something's not right.

pciehp_device_replaced() already checks whether device identity in
(unsigned) Config Space has changed after resume from system sleep
and after error recovery and automatically triggers a hot unplug event.
CMA will make this check even more useful because we can trigger
hot unplug also when the device was previously authenticated but now
no longer is, e.g. because of a Subject Alternative Name mismatch.

> > 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?

I think it doesn't really matter whether we use cached data or read it
from Config Space, we just want to detect a mismatch with the Subject
Alternative Name.

> > 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.

Right.

> > 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.

The assumption that pdev->class may exceed 6 hex characters
is a hallucination as the 24-bit size is prescribed by the spec.

Should the expected name ever change, the CMA_NAME_MAX macro
at the top of cma.c needs to be amended.  So I don't really
see the need for scnprintf().

> > > +     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.

Hallucination.

> > > +     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.

PCIe r7.0 sec 7.9.3.2 defines the Device Serial Number as an EUI-64.

In the corresponding IEEE spec on EUIs on page 11 in section
"Unassigned and NULL EUI values", it says:

   "Many applications have found it useful to define a distinct null
    identifier, most often indicating the absence of a valid EUI-48 or
    EUI-64 value. As an example, a null value might be the power-on state
    for an integrated circuit register, until the hardware or firmware
    initializes the register with a valid EUI.
    [...]
    The all-zeros EUI-48 value (00-00-00-00-00-00) and EUI-64 value
    (00-00-00- 00-00-00-00-00), though assigned to an organization,
    have not been and will not be used by that assignee as an EUI.
    (They can be considered as assigned to the IEEE Registration Authority.)"

    https://standards.ieee.org/content/dam/ieee-standards/standards/web/documents/tutorials/eui.pdf

PCIe r6.1 sec 6.31.3 says the Device Serial Number shall be included
in the Subject Alternative Name if it is "implemented".  It is debatable
whether it can be considered implemented if the vendor neglected to
initialize it to a valid number, but I'm inclined to consider that
"not implemented".  The vendor made a mistake and signed an invalid
Device Serial Number.  That can't be considered a valid certificate then.

> > 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?

Again, it doesn't matter whether the data is cached or read from
Config Space.  We just want to detect a mismatch with the Subject
Alternative Name.

Thanks,

Lukas