linux-kernel - Re: [PATCH v3 01/10] PCI: Avoid saving error values for config space

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d6655c44-ca97-4527-8788-94be2644c049@linux.ibm.com>
Date: Tue, 16 Sep 2025 13:00:30 -0700
From: Farhan Ali <alifm@...ux.ibm.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: linux-s390@...r.kernel.org, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        alex.williamson@...hat.com, schnelle@...ux.ibm.com,
        mjrosato@...ux.ibm.com
Subject: Re: [PATCH v3 01/10] PCI: Avoid saving error values for config space

On 9/16/2025 11:09 AM, Bjorn Helgaas wrote:
> On Thu, Sep 11, 2025 at 11:32:58AM -0700, Farhan Ali wrote:
>> The current reset process saves the device's config space state before
>> reset and restores it afterward. However, when a device is in an error
>> state before reset, config space reads may return error values instead of
>> valid data. This results in saving corrupted values that get written back
>> to the device during state restoration.
>>
>> Avoid saving the state of the config space when the device is in error.
>> While restoring we only restorei the state that can be restored through
>> kernel data such as BARs or doesn't depend on the saved state.
>>
>> Signed-off-by: Farhan Ali <alifm@...ux.ibm.com>
>> ---
>>   drivers/pci/pci.c      | 29 ++++++++++++++++++++++++++---
>>   drivers/pci/pcie/aer.c |  5 +++++
>>   drivers/pci/pcie/dpc.c |  5 +++++
>>   drivers/pci/pcie/ptm.c |  5 +++++
>>   drivers/pci/tph.c      |  5 +++++
>>   drivers/pci/vc.c       |  5 +++++
>>   6 files changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index b0f4d98036cd..4b67d22faf0a 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1720,6 +1720,11 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>>   	struct pci_cap_saved_state *save_state;
>>   	u16 *cap;
>>   
>> +	if (!dev->state_saved) {
>> +		pci_warn(dev, "Not restoring pcie state, no saved state");
>> +		return;
Hi Bjorn

Thanks for taking a look.

> Seems like a lot of messages.  If we want to warn about this, why
> don't we do it once in pci_restore_state()?

I thought providing messages about which state is not restored would be 
better and meaningful as we try to restore some of the state. But if the 
preference is to just have a single warn message in pci_restore_state 
then I can update it. (would also like to hear if Alex has any 
objections to that)

>
> I guess you're making some judgment about what things can be restored
> even when !dev->state_saved.  That seems kind of hard to maintain in
> the future as other capabilities are added.
>
> Also seems sort of questionable if we restore partial state and keep
> using the device as if all is well.  Won't the device be in some kind
> of inconsistent, unpredictable state then?
>
> Bjorn

I tried to avoid restoring state that explicitly needed to save the 
state. For some of the other capabilities, that didn't explicitly store 
the state, I tried to keep the same behavior. This is based on the 
discussion with Alex 
(https://lore.kernel.org/all/20250826094845.517e0fa7.alex.williamson@redhat.com/). 
Also AFAIU currently the dev->state_saved is set to true as long as we 
save the first 64 bytes of config space (pci_save_state), so we could 
for example fail to save the PCIe state, but while restoring can 
continue to restore other capabilities like pasid.

At the very least I would like to avoid corrupting the BAR registers and 
restore msix (arch_restore_msi_irqs) to get devices into a functional 
state after a reset. I am open to suggestions on how we can do this.

Would also like to get your feedback on patch 3 and the approach there 
of having a new flag in struct pci_slot.

Thanks
Farhan