[PATCH 0/2] PCI: Fix NULL pointer access in pci_store_saved_state()

[PATCH 0/2] PCI: Fix NULL pointer access in pci_store_saved_state()

[Krishna Chaitanya Chundru]

If the PCIe link goes down while pci_save_state() is in progress, reads
from the device configuration space may return invalid values(all 0xF's).

This can lead to saving corrupted or inconsistent capability state and
subsequent memory corruption. The issue is not limited to a specific
capability type and may occur at any point during the save process.

One example is, while saving VC extended capability save path
(pci_save_vc_state() / pci_vc_do_save_buffer()) then interprets all-1s
capability fields as valid and ends up writing far beyond the allocated
pci_cap_saved_state buffer, corrupting the pci_dev->saved_cap_space list.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
Krishna Chaitanya Chundru (2):
      PCI: Add pcie_link_is_active() to determine if the link is active
      PCI: Fix NULL pointer access in pci_store_saved_state()

 drivers/pci/hotplug/pciehp.h      |  1 -
 drivers/pci/hotplug/pciehp_ctrl.c |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c  | 35 ++++-------------------------------
 drivers/pci/pci.c                 | 38 +++++++++++++++++++++++++++++++++++---
 drivers/pci/pci.h                 |  1 +
 5 files changed, 41 insertions(+), 36 deletions(-)
---
base-commit: 7ca6d1cfec80ebe46cc063f3284c5896c344d9a1
change-id: 20260303-fix_pci_access-c03b3b64ddbc

Best regards,
-- 
Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>

[PATCH 1/2] PCI: Add pcie_link_is_active() to determine if the link is active

[Krishna Chaitanya Chundru]

Add pcie_link_is_active() a common API to check if the PCIe link is active,
replacing duplicate code in multiple locations.

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Signed-off-by: Timothy Pearson <tpearson@raptorengineering.com>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
---
This patch is already submitted as part of different series, but this
is dropped from the series. we want to bring this now, to check null
point access issue at pci_store_saved_state() due to linkdown.
Link to previous patch: https://lore.kernel.org/all/20250828-qps615_v4_1-v6-7-985f90a7dd03@oss.qualcomm.com/
---
 drivers/pci/hotplug/pciehp.h      |  1 -
 drivers/pci/hotplug/pciehp_ctrl.c |  2 +-
 drivers/pci/hotplug/pciehp_hpc.c  | 35 ++++-------------------------------
 drivers/pci/pci.c                 | 28 +++++++++++++++++++++++++---
 drivers/pci/pci.h                 |  1 +
 5 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index debc79b0adfb2c8e06aabb765e1741572685100b..79df49cc99463829f563db1dc8014a51ccfac0af 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -186,7 +186,6 @@ int pciehp_query_power_fault(struct controller *ctrl);
 int pciehp_card_present(struct controller *ctrl);
 int pciehp_card_present_or_link_active(struct controller *ctrl);
 int pciehp_check_link_status(struct controller *ctrl);
-int pciehp_check_link_active(struct controller *ctrl);
 bool pciehp_device_replaced(struct controller *ctrl);
 void pciehp_release_ctrl(struct controller *ctrl);
 
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index 7805f697a02ceab33cc962587e0ad85c16c0d962..e165e6d810214f26451920976b1f11d32dd162aa 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -269,7 +269,7 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 	/* Turn the slot on if it's occupied or link is up */
 	mutex_lock(&ctrl->state_lock);
 	present = pciehp_card_present(ctrl);
-	link_active = pciehp_check_link_active(ctrl);
+	link_active = pcie_link_is_active(ctrl->pcie->port);
 	if (present <= 0 && link_active <= 0) {
 		if (ctrl->state == BLINKINGON_STATE) {
 			ctrl->state = OFF_STATE;
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 4c62140a3cb444b1d29a378099d4c3d377b93d15..abfae48470ce44329d2bf84469e826ab4dea99af 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -221,33 +221,6 @@ static void pcie_write_cmd_nowait(struct controller *ctrl, u16 cmd, u16 mask)
 	pcie_do_write_cmd(ctrl, cmd, mask, false);
 }
 
-/**
- * pciehp_check_link_active() - Is the link active
- * @ctrl: PCIe hotplug controller
- *
- * Check whether the downstream link is currently active. Note it is
- * possible that the card is removed immediately after this so the
- * caller may need to take it into account.
- *
- * If the hotplug controller itself is not available anymore returns
- * %-ENODEV.
- */
-int pciehp_check_link_active(struct controller *ctrl)
-{
-	struct pci_dev *pdev = ctrl_dev(ctrl);
-	u16 lnk_status;
-	int ret;
-
-	ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
-	if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
-		return -ENODEV;
-
-	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
-	ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
-
-	return ret;
-}
-
 static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
 {
 	u32 l;
@@ -468,7 +441,7 @@ int pciehp_card_present_or_link_active(struct controller *ctrl)
 	if (ret)
 		return ret;
 
-	return pciehp_check_link_active(ctrl);
+	return pcie_link_is_active(ctrl_dev(ctrl));
 }
 
 int pciehp_query_power_fault(struct controller *ctrl)
@@ -615,8 +588,8 @@ static void pciehp_ignore_link_change(struct controller *ctrl,
 	 * Synthesize it to ensure that it is acted on.
 	 */
 	down_read_nested(&ctrl->reset_lock, ctrl->depth);
-	if (!pciehp_check_link_active(ctrl) || pciehp_device_replaced(ctrl))
-		pciehp_request(ctrl, ignored_events);
+	if (!pcie_link_is_active(pdev) || pciehp_device_replaced(ctrl))
+		pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
 	up_read(&ctrl->reset_lock);
 }
 
@@ -922,7 +895,7 @@ int pciehp_slot_reset(struct pcie_device *dev)
 	pcie_capability_write_word(dev->port, PCI_EXP_SLTSTA,
 				   PCI_EXP_SLTSTA_DLLSC);
 
-	if (!pciehp_check_link_active(ctrl))
+	if (!pcie_link_is_active(ctrl_dev(ctrl)))
 		pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
 
 	return 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8479c2e1f74f1044416281aba11bf071ea89488a..1488c93d4e22371480165cb55afc7a0c3cae238e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4756,7 +4756,6 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
 		return 0;
 
 	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
-		u16 status;
 
 		pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
 		msleep(delay);
@@ -4772,8 +4771,7 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
 		if (!dev->link_active_reporting)
 			return -ENOTTY;
 
-		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
-		if (!(status & PCI_EXP_LNKSTA_DLLLA))
+		if (pcie_link_is_active(dev) <= 0)
 			return -ENOTTY;
 
 		return pci_dev_wait(child, reset_type,
@@ -6116,6 +6114,30 @@ void pcie_print_link_status(struct pci_dev *dev)
 }
 EXPORT_SYMBOL(pcie_print_link_status);
 
+/**
+ * pcie_link_is_active() - Checks if the link is active or not
+ * @pdev: PCI device to query
+ *
+ * Check whether the downstream link is currently active. Note it is
+ * possible that the card is removed immediately after this so the
+ * caller may need to take it into account.
+ *
+ * Return: true if link is active, or -ENODEV if the config read fails.
+ */
+int pcie_link_is_active(struct pci_dev *pdev)
+{
+	u16 lnk_status;
+	int ret;
+
+	ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
+	if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
+		return -ENODEV;
+
+	pci_dbg(pdev, "lnk_status = %#06x\n", lnk_status);
+	return !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
+}
+EXPORT_SYMBOL(pcie_link_is_active);
+
 /**
  * pci_select_bars - Make BAR mask from the type of resource
  * @dev: the PCI device for which BAR mask is made
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 13d998fbacce6698514d92500dfea03cc562cdc2..c9a6e5d3de3aeab125d2e456456359aa857e7a19 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -355,6 +355,7 @@ static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
 /* Functions for PCI Hotplug drivers to use */
 int pci_hp_add_bridge(struct pci_dev *dev);
 bool pci_hp_spurious_link_change(struct pci_dev *pdev);
+int pcie_link_is_active(struct pci_dev *dev);
 
 #if defined(CONFIG_SYSFS) && defined(HAVE_PCI_LEGACY)
 void pci_create_legacy_files(struct pci_bus *bus);

-- 
2.34.1

[PATCH 2/2] PCI: Fix NULL pointer access in pci_store_saved_state()

[Krishna Chaitanya Chundru]

If the PCIe link goes down while pci_save_state() is in progress, reads
from the device configuration space may return invalid values(all 0xF's).

This can lead to saving corrupted or inconsistent capability state and
subsequent memory corruption. The issue is not limited to a specific
capability type and may occur at any point during the save process.

One example is, while saving VC extended capability save path
(pci_save_vc_state() / pci_vc_do_save_buffer()) then interprets all-1s
capability fields as valid and ends up writing far beyond the allocated
pci_cap_saved_state buffer, corrupting the pci_dev->saved_cap_space list.

The call stack of the issue as follows.

[ T1634] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000013
[ T1634] Mem abort info:
[ T1634]   ESR = 0x96000005
[ T1634]   EC = 0x25: DABT (current EL), IL = 32 bits
[ T1634]   SET = 0, FnV = 0
[ T1634]   EA = 0, S1PTW = 0
[ T1634]   FSC = 0x05: level 1 translation fault
[ T1634] Data abort info:
[ T1634]   ISV = 0, ISS = 0x00000005
[ T1634]   CM = 0, WnR = 0
[ T1634] user pgtable: 4k pages, 39-bit VAs, pgdp=00000000ac2ed000
[ T1634] [0000000000000013] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
[ T1634] Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
[ T1634] Dumping ftrace buffer:
[ T1634]    (ftrace buffer empty)
[ T1634] pc : pci_store_saved_state+0x40/0xd8
[ T1634] lr : cnss_set_pci_config_space+0x54/0x100 [cnss2]
[ T1634] Call trace:
[ T1634]  pci_store_saved_state+0x40/0xd8
[ T1634]  cnss_set_pci_config_space+0x54/0x100 [cnss2]

Fix this issue by bailing out early from pci_store_saved_state() if link
is not active and also make saved_state = false.

The link state check here is racy since the link may transition at any
time. This is a best-effort attempt to avoid saving PCI state when the
link is already down.

Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1488c93d4e22371480165cb55afc7a0c3cae238e..06bd6b7d62ec1a41bd12af2ab47ecd2b77665c7e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1875,6 +1875,16 @@ struct pci_saved_state *pci_store_saved_state(struct pci_dev *dev)
 	if (!dev->state_saved)
 		return NULL;
 
+	/*
+	 * The link state check here is racy since the link may transition at
+	 * any time. This is a best-effort attempt to avoid saving PCI state
+	 * when the link is already down.
+	 */
+	if (!pcie_link_is_active(dev)) {
+		dev->state_saved = false;
+		return NULL;
+	}
+
 	size = sizeof(*state) + sizeof(struct pci_cap_saved_data);
 
 	hlist_for_each_entry(tmp, &dev->saved_cap_space, next)

-- 
2.34.1

Re: [PATCH 2/2] PCI: Fix NULL pointer access in pci_store_saved_state()

[Lukas Wunner]

On Sat, Apr 04, 2026 at 02:23:00PM +0530, Krishna Chaitanya Chundru wrote:
> If the PCIe link goes down while pci_save_state() is in progress, reads
> from the device configuration space may return invalid values(all 0xF's).

That should be harmless.  If the link goes down, the device should
subsequently be de-enumerated by the hotplug driver.  If we save
some bogus data before de-enumerating it, so be it.

If the port above is not hotplug-capable, manual intervention is
required for remove/rescan, but that's orthogonal to this problem.

> One example is, while saving VC extended capability save path
> (pci_save_vc_state() / pci_vc_do_save_buffer()) then interprets all-1s
> capability fields as valid and ends up writing far beyond the allocated
> pci_cap_saved_state buffer, corrupting the pci_dev->saved_cap_space list.

I'm not familiar with drivers/pci/vc.c, but it seems it takes a size
read from config space and uses it to write to a particular memory
area?  That feels totally wrong, at the very least there should be
a check for PCI_POSSIBLE_ERROR().

> The link state check here is racy since the link may transition at any
> time. This is a best-effort attempt to avoid saving PCI state when the
> link is already down.

No, please validate values read from config space with
PCI_POSSIBLE_ERROR() before using them to access memory at
a location that may be out-of-bounds.  Or cache the size on
enumeration and avoid re-reading it upon pci_save_state().

> +	/*
> +	 * The link state check here is racy since the link may transition at
> +	 * any time. This is a best-effort attempt to avoid saving PCI state
> +	 * when the link is already down.
> +	 */
> +	if (!pcie_link_is_active(dev)) {
> +		dev->state_saved = false;
> +		return NULL;
> +	}

The state_saved flag is only used by PM code to determine whether
the driver called pci_save_state().  If it didn't, the PCI core
will make up for it by calling pci_save_state().

The state_saved flag is *not* an indication whether the saved state
is usable and code outside power management has no business changing
the flag's value.

Thanks,

Lukas