Re: [PATCH v2 2/7] PCI/parisc: Clean up resource debug print & use dynamic debug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: uninitialized stack pointer dereference in pcibios_align_resource() via %pR
--

commit e687046bde97f65f6b278cb7b13dd117863bc565
Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

PCI/parisc: Clean up resource debug print & use dynamic debug

This commit cleans up the ad-hoc RES_DBG() macro in the parisc PCI code
by replacing it with the standard pci_dbg() macro. It also updates the
format string to use %pR for printing resources, which matches the typical
format used by the PCI core.

> diff --git a/arch/parisc/kernel/pci.c b/arch/parisc/kernel/pci.c
> index b8007c7400d47..518f532ccddc5 100644
> --- a/arch/parisc/kernel/pci.c
> +++ b/arch/parisc/kernel/pci.c

[ ... ]

> @@ -204,10 +196,9 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>  	struct pci_dev *dev = data;
>  	resource_size_t align, start = res->start;
>  
> -	DBG_RES("pcibios_align_resource(%s, (%p) [%lx,%lx]/%x, 0x%lx, 0x%lx)\n",
> -		pci_name(((struct pci_dev *) data)),
> -		res->parent, res->start, res->end,
> -		(int) res->flags, size, alignment);
> +	pci_dbg(dev, "%pR: pcibios_align_resource(%lx, 0x%llx, 0x%llx), parent %pR\n",
> +		res, res->flags, (unsigned long long)size,
> +		(unsigned long long)alignment, res->parent);

[Severity: High]
Could this format string introduce a regression by causing a kernel oops due
to an uninitialized pointer dereference?

The %pR format specifier causes the printk implementation to dereference the
resource pointer to read its boundaries and flags. However,
pcibios_align_resource() is used as an alignf callback for resource
allocation, and it can be called with a partially uninitialized stack
variable from __find_resource_space():

kernel/resource.c:__find_resource_space() {
	...
	struct resource avail;
	...
	avail.start = ALIGN(full_avail.start, constraint->align);
	avail.end = full_avail.end;
	avail.flags = new->flags;
	if (avail.start >= full_avail.start) {
		alloc.flags = avail.flags;
		if (alignf) {
			alloc.start = alignf(constraint->alignf_data,
					     &avail, &full_avail,
					     size, constraint->align);
	...
}

In this path, the avail.parent field is left uninitialized. When %pR attempts
to dereference res->parent, it will be reading stack garbage. The previous
DBG_RES() implementation used %p, which safely printed the pointer address
without dereferencing it.

Would it be safer to revert to %p for the parent pointer or ensure the
struct is fully initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002113319.6652-1-ilpo.jarvinen@linux.intel.com?part=2