Re: [PATCH v16 1/7] PCI: Allow per function PCI slots to fix slot reset on s390

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] pci_dev_matches_slot() breaks PCI_SLOT_ALL_DEVICES matching logic on architectures with per_func_slot enabled.
- [High] Unconditional enablement of per_func_slot bypasses generic PCI multifunction reset protections, breaking device isolation.
- [High] Lockless access to dev->slot in pci_dev_reset_slot_function() causes a Time-Of-Check to Time-Of-Use (TOCTOU) Use-After-Free (UAF).
- [Medium] The commit message inaccurately describes a memory leak in pci_slot_release(), obscuring a real Use-After-Free vulnerability.
--

commit d10e79ced185e01a7483fe5611e52ccd5f7f1f65
Author: Farhan Ali <alifm@linux.ibm.com>

PCI: Allow per function PCI slots to fix slot reset on s390

On s390 systems, PCI devices are always accessed through a form of PCI
pass-through which operates on a per PCI function granularity. Add a flag
for struct pci_slot to allow per function PCI slots for functions managed
through a hypervisor.

> When attempting to reset a function through the hotplug driver, the shared
> slot assignment causes the wrong function to be reset instead of the
> intended one. It also leaks memory as we do create a pci_slot object for
> the function, but don't correctly free it in pci_slot_release().

The commit message states that it leaks memory because the pci_slot object
isn't freed in pci_slot_release(). Is this description accurate?

Looking at pci_slot_release(), it unconditionally calls kfree(slot), so the
object is freed.

However, because the old matching logic failed, dev->slot wasn't set to NULL.
Does this leave a dangling pointer and a use-after-free vulnerability rather
than a memory leak?

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc000901..d0c9f0166af5c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4865,8 +4865,9 @@ static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, bool probe)
>  
>  static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
>  {
> -	if (dev->multifunction || dev->subordinate || !dev->slot ||
> -	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
> +	if (dev->subordinate || !dev->slot ||
> +	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
> +	    (dev->multifunction && !dev->slot->per_func_slot))

Does accessing dev->slot->per_func_slot without locks introduce a race
condition?

If a hotplug controller is concurrently unbound, pci_slot_release() could
free the slot memory. Since this path doesn't hold pci_slot_mutex or
a kobject reference to the slot, could dev->slot be freed right after
checking !dev->slot, causing a use-after-free when accessing per_func_slot?

>  		return -ENOTTY;
>  
>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 6d5cd37bfb1e7..894d6213ed306 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -72,6 +72,23 @@ static ssize_t cur_speed_read_file(struct pci_slot *slot, char *buf)
>  	return bus_speed_read(slot->bus->cur_bus_speed, buf);
>  }
>  
> +static bool pci_dev_matches_slot(struct pci_dev *dev, struct pci_slot *slot)
> +{
> +	if (slot->per_func_slot)
> +		return dev->devfn == slot->number;
> +
> +	return slot->number == PCI_SLOT_ALL_DEVICES ||
> +		PCI_SLOT(dev->devfn) == slot->number;
> +}

Does the early return for slot->per_func_slot break device-to-slot binding
for generic hotplug drivers?

If a driver registers a slot using PCI_SLOT_ALL_DEVICES, the check
dev->devfn == 0xfeff will unconditionally fail since devfn is an 8-bit
integer.

Additionally, if a generic hotplug driver registers a slot with a specific
device number, this strict equality check will only match Function 0 and
ignore Functions 1-7 of a multifunction device.

Should the PCI_SLOT_ALL_DEVICES check take precedence over the per_func_slot
logic?

> +static bool pci_slot_enabled_per_func(void)
> +{
> +	if (IS_ENABLED(CONFIG_S390))
> +		return true;
> +
> +	return false;
> +}

[ ... ]

> @@ -298,6 +313,9 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  	slot->bus = pci_bus_get(parent);
>  	slot->number = slot_nr;
>  
> +	if (pci_slot_enabled_per_func())
> +		slot->per_func_slot = 1;

By globally forcing per_func_slot to 1 on S390, does this assume all
hotplug slots on the architecture are managed by the s390_pci_hpc driver?

If standard PCIe hotplug drivers like pciehp are used (e.g., inside KVM
guests), they will also be marked with per_func_slot = 1.

This would bypass the multifunction protection in
pci_dev_reset_slot_function(), potentially allowing a slot reset request to
trigger a full secondary bus reset. Could this forcefully reset other
functions on the device owned by the host or other guests?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514180059.1553-1-alifm@linux.ibm.com?part=1