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

[sashiko-bot@kernel.org]

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

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

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

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

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.

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.

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?

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

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508031710.514574-1-alistair.francis@wdc.com?part=10