Re: [PATCH v5 02/11] PCI: liveupdate: Track outgoing preserved PCI devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Dangling pointer in dev->liveupdate.outgoing causes Use-After-Free memory corruption and state leaks if a device is not properly unpreserved.
- [High] Missing `kho_is_enabled()` check in `pci_flb_preserve()` violates KHO subsystem guidelines.
--

commit f946f5d8b0614325899345e29b5315ff6d5139e9
Author: David Matlack <dmatlack@google.com>

PCI: liveupdate: Track outgoing preserved PCI devices

This patch adds APIs to allow drivers to notify the PCI core of which devices
are being preserved across a Live Update. It allocates a struct pci_ser via
the KHO subsystem to track the outgoing preserved devices.

> diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
> --- a/drivers/pci/liveupdate.c
> +++ b/drivers/pci/liveupdate.c

[ ... ]

> @@ -74,27 +110,30 @@ static int pci_flb_preserve(struct liveupdate_flb_op_args *args)
>  
>  	size = struct_size_t(struct pci_ser, devices, max_nr_devices);
>  
> -	ser = kho_alloc_preserve(size);
> -	if (IS_ERR(ser))
> -		return PTR_ERR(ser);
> +	outgoing->ser = kho_alloc_preserve(size);

Does this allocation need to be gated by kho_is_enabled()?

If KHO is disabled, kho_alloc_preserve() will silently add tracking state
that will never be used.

> +	if (IS_ERR(outgoing->ser)) {
> +		kfree(outgoing);
> +		return PTR_ERR(outgoing->ser);
> +	}
>  
>  	pr_debug("Preserved struct pci_ser with room for %u devices\n",
>  		 max_nr_devices);

[ ... ]

>  static void pci_flb_unpreserve(struct liveupdate_flb_op_args *args)
>  {
> -	struct pci_ser *ser = args->obj;
> +	struct pci_flb_outgoing *outgoing = args->obj;
>  
> -	WARN_ON_ONCE(ser->nr_devices);
> -	kho_unpreserve_free(ser);
> +	WARN_ON_ONCE(outgoing->ser->nr_devices);
> +	kho_unpreserve_free(outgoing->ser);
> +	kfree(outgoing);

If a driver fails to unpreserve a device before the FLB is torn down, this
WARN_ON_ONCE triggers, but the memory is still freed.

Does this leave dev->liveupdate.outgoing pointing to freed memory?

>  
>  	pr_debug("Unpreserved struct pci_ser\n");
>  }

[ ... ]

> +void pci_liveupdate_unpreserve(struct pci_dev *dev)
> +{
> +	struct pci_flb_outgoing *outgoing = NULL;
> +	struct pci_dev_ser *dev_ser;
> +	struct pci_ser *ser;
> +	int ret;
> +
> +	ret = liveupdate_flb_get_outgoing(&pci_liveupdate_flb, (void **)&outgoing);
> +
> +	if (ret || !outgoing) {
> +		pci_warn(dev, "Cannot unpreserve device without outgoing Live Update state\n");
> +		return;
> +	}
> +
> +	guard(mutex)(&outgoing->lock);
> +	ser = outgoing->ser;
> +
> +	guard(write_lock)(&dev->liveupdate.lock);
> +
> +	dev_ser = dev->liveupdate.outgoing;
> +	if (!dev_ser) {
> +		pci_warn(dev, "Cannot unpreserve device that is not preserved\n");
> +		return;
> +	}
> +
> +	pci_info(dev, "Device will no longer be preserved across next Live Update\n");
> +	ser->nr_devices--;
> +	memset(dev_ser, 0, sizeof(*dev_ser));

If dev->liveupdate.outgoing was left dangling from a previous torn-down
session, can this corrupt memory?

If a new Live Update session starts, pci_liveupdate_unpreserve() would
fetch the new outgoing FLB but operate on the old dangling
dev->liveupdate.outgoing pointer.

Does this code cause a use-after-free in the memset() here, as well as
corrupt the new session's counter when decrementing ser->nr_devices?

> +	dev->liveupdate.outgoing = NULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512184846.119396-1-dmatlack@google.com?part=2