Re: [PATCH v2] pci_crash: capture PCI config space at panic time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Calling pci_read_config_dword() in the panic path can cause a hard deadlock if another CPU was halted while holding the pci_lock raw spinlock.
- [High] pci_crash_build_pagemap() discards the intra-page offset for kvmalloc buffers, exposing garbage data to the crash parser.
- [High] Lack of RCU synchronization and memory barriers during rebuild allows the panic path to hit use-after-free or out-of-bounds array accesses.
- [High] Accessing the PCI configuration space of devices in the D3cold power state triggers fatal bus errors.
- [Medium] Missing mutex in capture_param_set() allows concurrent sysfs writes to corrupt the configuration string.
- [Medium] The default capture mode in code contradicts the documented default in the commit message.
- [Medium] Rebuild incorrectly retains stale state and leaks device references if zero devices match the filter.
- [Low] Ignoring the return value of strim() fails to remove leading whitespace from module parameters.
- [Low] Registering the bus notifier after the initial snapshot creates a race condition that can miss hotplug events.
--

commit 0e9239796d3fc7bf4512456a275c5174281a6334
Author: Johannes Hange <hangej@amazon.com>
pci_crash: capture PCI config space at panic time

This commit adds CONFIG_PCI_CRASH to capture PCI configuration space
(including AER registers) during a panic. The data is written to a
pre-allocated buffer and exported via VMCOREINFO for crash analysis.

Link: https://lore.kernel.org/oe-kbuild-all/202605140501.1tSG7lxR-lkp@intel.com/

> - capture=aer (default): quick-scan root port ROOT_STATUS, bail if
>   no uncorrectable errors.  capture=always: every panic.

[Severity: Medium]
This isn't a bug in the code, but the commit message states the default
capture mode is aer, while the code initializes it to always. Should
the documentation and code match?

> diff --git a/kernel/pci_crash.c b/kernel/pci_crash.c
> --- /dev/null
> +++ b/kernel/pci_crash.c

[ ... ]

> +static int capture_param_set(const char *val, const struct kernel_param *kp)
> +{
> +	if (strlen(val) >= sizeof(capture))
> +		return -EINVAL;
> +	strscpy(capture, val, sizeof(capture));
> +	strim(capture);
> +	if (READ_ONCE(pci_crash_ready))
> +		pci_crash_parse_capture();
> +	return 0;
> +}

[Severity: Medium]
Is it possible for concurrent sysfs writes to corrupt the configuration string
since this locklessly modifies the global capture array?

[Severity: Low]
Does ignoring the return value of strim() here (and in devices_param_set)
leave leading spaces in the buffer, causing exact string matches (like 
strcmp(token, "aer")) to fail later?

[ ... ]

> +static struct pci_crash_pagemap *pci_crash_build_pagemap(void *buf,
> +							 size_t buf_size)
> +{
> +	unsigned int num_pages = DIV_ROUND_UP(buf_size, PAGE_SIZE);
> +	struct pci_crash_pagemap *pm;
> +	unsigned int i;
> +
> +	pm = kmalloc(struct_size(pm, addrs, num_pages), GFP_KERNEL);
> +	if (!pm)
> +		return NULL;
> +
> +	pm->magic = cpu_to_le32(PCI_CRASH_PAGEMAP_MAGIC);
> +	pm->num_pages = cpu_to_le32(num_pages);
> +	pm->buf_size = cpu_to_le64(buf_size);
> +
> +	for (i = 0; i < num_pages; i++) {
> +		struct page *page;
> +		phys_addr_t pa;
> +
> +		if (is_vmalloc_addr(buf + i * PAGE_SIZE))
> +			page = vmalloc_to_page(buf + i * PAGE_SIZE);
> +		else
> +			page = virt_to_page(buf + i * PAGE_SIZE);
> +
> +		if (!page) {
> +			kfree(pm);
> +			return NULL;
> +		}
> +		pa = page_to_phys(page);

[Severity: High]
Could this discard the intra-page offset for kvmalloc buffers? When kvmalloc
falls back to kmalloc for small sizes, the returned buffer might not be
page-aligned. Would calculating the physical address from the page boundary
cause the vmcore parser to read unrelated slab data and miss the
PCI_CRASH_MAGIC header?

[ ... ]

> +static void pci_crash_read_config_space(struct pci_dev *pdev, u8 *ptr)
> +{
> +	struct pci_crash_device_record *record =
> +		(struct pci_crash_device_record *)ptr;
> +	u8 *cfg_data = ptr + PCI_CRASH_RECORD_META;
> +	int offset;
> +	u32 val;
> +
> +	record->domain = cpu_to_le16(pci_domain_nr(pdev->bus));
> +	record->bus = pdev->bus->number;
> +	record->devfn = pdev->devfn;
> +	record->config_size = cpu_to_le32(pdev->cfg_size);
> +
> +	for (offset = 0; offset < pdev->cfg_size; offset += 4) {
> +		if (pci_read_config_dword(pdev, offset, &val)) {
> +			put_unaligned_le32(0xFFFFFFFF, &cfg_data[offset]);
> +			continue;
> +		}

[Severity: High]
Will accessing the configuration space of devices in the D3cold power state
trigger fatal bus errors? Unconditional MMIO reads to a physically powered-off
device can cause unhandled Master/Target Aborts, potentially escalating to
fatal exceptions (MCE/SError) that immediately double-fault the panic path.

[ ... ]

> +static void pci_crash_rebuild_snapshot(void)
> +{
> +	struct pci_dev *pdev = NULL;
> +	unsigned int count = 0, i;

[ ... ]

> +	/*
> +	 * Pass 2: populate device array (filtered) and compute exact
> +	 * buffer size from actual pdev->cfg_size per device.
> +	 * count from pass 1 is an upper bound; actual may be smaller.
> +	 */
> +	total_size = PCI_CRASH_HEADER_SIZE;
> +	pdev = NULL;
> +	i = 0;
> +	for_each_pci_dev(pdev) {
> +		if (i >= count) {
> +			pci_dev_put(pdev);
> +			break;
> +		}
> +		if (!pci_crash_device_matches(pdev))
> +			continue;
> +		new_devs[i] = pci_dev_get(pdev);
> +		total_size += PCI_CRASH_RECORD_META + pdev->cfg_size;
> +		i++;
> +	}
> +	count = i;
> +
> +	if (count == 0) {
> +		kvfree(new_devs);
> +		pr_info("no devices match devices=%s\n", devices);
> +		goto err_restore;
> +	}

[Severity: Medium]
If a hotplug removal or filter change results in zero matching devices, this
jumps to err_restore and restores the old count, keeping the old_devs array
intact. Does this leak the refcounts of the removed devices and continue to
expose stale device data in the crash buffer?

[ ... ]

> +	/* Release old device references */
> +	for (i = 0; i < old_num_devs; i++)
> +		if (old_devs && old_devs[i])
> +			pci_dev_put(old_devs[i]);
> +
> +	WRITE_ONCE(pci_crash_devs, new_devs);
> +	kvfree(old_devs);
> +
> +	/*
> +	 * Swap pagemap first (with pre-computed phys addr), then buffer,
> +	 * then size and count.  If crash fires mid-swap, num_devs is
> +	 * still 0 from above, so pci_crash_save() bails out safely.
> +	 */
> +	WRITE_ONCE(pci_crash_pagemap, new_pm);
> +	WRITE_ONCE(pci_crash_pagemap_phys, virt_to_phys(new_pm));
> +	WRITE_ONCE(pci_crash_pagemap_size, struct_size(new_pm, addrs,
> +			le32_to_cpu(new_pm->num_pages)));
> +	kfree(old_pm);
> +
> +	old_buf = pci_crash_buffer;
> +	WRITE_ONCE(pci_crash_buffer, new_buf);
> +	WRITE_ONCE(pci_crash_buffer_size, total_size);
> +	/* Ensure buffer/pagemap/size are visible before num_devs enables capture */
> +	smp_wmb();
> +	WRITE_ONCE(pci_crash_num_devs, count);
> +	kvfree(old_buf);

[Severity: High]
Does the lack of RCU synchronization and memory barriers here allow the panic
path to hit a use-after-free or out-of-bounds array access? 

The old_devs array is freed immediately after updating the pointer, and
new_devs can be smaller. A panicking CPU could locklessly read a stale,
larger pci_crash_num_devs and then iterate over the new smaller array, or
iterate over old_devs while it is being freed.

[ ... ]

> +void pci_crash_save(void)
> +{
> +	struct pci_crash_pagemap *pm;
> +	unsigned long cflags;
> +	unsigned int num_devs;
> +	size_t pm_size;
> +	size_t buf_size;
> +	void *buffer;

[ ... ]

> +#ifdef CONFIG_PCIEAER
> +		if (cflags & PCI_CRASH_CAPTURE_AER) {
> +			struct pci_dev **devs = READ_ONCE(pci_crash_devs);
> +			unsigned int i;
> +			bool pci_error_found = false;
> +
> +			if (!devs)
> +				return;
> +
> +			for (i = 0; i < num_devs; i++) {
> +				struct pci_dev *pdev = devs[i];
> +				u32 status = 0;
> +
> +				if (!pdev || !pdev->aer_cap)
> +					continue;
> +
> +				if (pci_pcie_type(pdev) == PCI_EXP_TYPE_ROOT_PORT) {
> +					pci_read_config_dword(pdev,
> +							     pdev->aer_cap + PCI_ERR_ROOT_STATUS,
> +							     &status);

[Severity: High]
Can calling pci_read_config_dword() in the panic path cause a hard deadlock?
This function acquires the global pci_lock raw spinlock. If another CPU was
forcefully halted by an NMI while holding this lock, the panicking CPU will
spin indefinitely and completely hang the crash dump process.

[ ... ]

> +static int __init pci_crash_init(void)
> +{
> +	/* Nothing to do in crash kernel -- the buffer from the first kernel
> +	 * is already in RAM (flushed before kexec) and the parser finds it
> +	 * via the pagemap in VMCOREINFO.
> +	 */
> +	if (is_kdump_kernel())
> +		return 0;
> +
> +	INIT_DELAYED_WORK(&pci_crash_rebuild_dwork, pci_crash_rebuild_worker);
> +
> +	pci_crash_parse_capture();
> +	pci_crash_parse_devices();
> +
> +	mutex_lock(&pci_crash_lock);
> +	pci_crash_rebuild_snapshot();
> +	mutex_unlock(&pci_crash_lock);
> +
> +	bus_register_notifier(&pci_bus_type, &pci_crash_bus_nb);

[Severity: Low]
Does registering the bus notifier after the initial snapshot create a race
condition? Any PCI hotplug events occurring in the window between the
snapshot completion and the notifier registration might be missed, leaving
the crash buffer out of sync with the hardware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521223010.2681515-1-hangej@amazon.com?part=1