| 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
| ||
|
Message-ID: <659b8389-16a7-423b-a231-5489c7cc0da9@163.com>
Date: Sat, 2 Aug 2025 00:54:27 +0800
From: Hans Zhang <18255117159@....com>
To: Gerd Bayer <gbayer@...ux.ibm.com>, Manivannan Sadhasivam
<mani@...nel.org>, Hans Zhang <hans.zhang@...tech.com>
Cc: Arnd Bergmann <arnd@...nel.org>, Bjorn Helgaas <helgaas@...nel.org>,
bhelgaas@...gle.com, Alexander Gordeev <agordeev@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
jingoohan1@...il.com, Krzysztof Wilczyński
<kwilczynski@...nel.org>, linux-kernel@...r.kernel.org,
linux-s390@...r.kernel.org, linux-next <linux-next@...r.kernel.org>,
linux-pci@...r.kernel.org, Lorenzo Pieralisi <lpieralisi@...nel.org>,
Rob Herring <robh@...nel.org>, Niklas Schnelle <schnelle@...ux.ibm.com>,
geert@...ux-m68k.org
Subject: Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()
On 2025/8/1 19:30, Gerd Bayer wrote:
> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>
> <--- snip --->
>
>>>>>>> The pci_bus_read_config() interface itself may have been a
>>>>>>> mistake, can't the callers just use the underlying helpers
>>>>>>> directly?
>>>>>>>
>>>>>>
>>>>>> They can! Since the callers of this API is mostly the macros, we can easily
>>>>>> implement the logic to call relevant accessors based on the requested size.
>>>>>>
>>>>>> Hans, could you please respin the series based the feedback since the series is
>>>>>> dropped for 6.17.
>>>>>>
>>>>>
>>>>> Dear all,
>>>>>
>>>>> I am once again deeply sorry for the problems that occurred in this series.
>>>>> I only test pulling the ARM platform.
>>>>>
>>>>> Thank you very much, Gerd, for reporting the problem.
>
> no worries!
>
>>>>> Thank you all for your discussions and suggestions for revision.
>>>>>
>>>>> Hi Mani,
>>>>>
>>>>> Geert provided a solution. My patch based on this is as follows. Please
>>>>> check if there are any problems.
>>>>> https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
>>>>>
>>>>> Also, please ask Gerd to help test whether it works properly. Thank you very
>>>>> much.
>>>>>
>
> I found Geert's proposal intriguing for a quick resolution of the
> issue. Yet, I have not tried that proposal, though.
>
Hi Gerd,
As I mentioned in my reply to Mani's email, the data ultimately read
here is also a forced type conversion.
#define PCI_OP_READ(size, type, len) \
int noinline pci_bus_read_config_##size \
(struct pci_bus *bus, unsigned int devfn, int pos, type *value) \
{ \
unsigned long flags; \
u32 data = 0; \
int res; \
\
if (PCI_##size##_BAD) \
return PCIBIOS_BAD_REGISTER_NUMBER; \
\
pci_lock_config(flags); \
res = bus->ops->read(bus, devfn, pos, len, &data); \
if (res) \
PCI_SET_ERROR_RESPONSE(value); \
else \
*value = (type)data; \
pci_unlock_config(flags); \
\
return res; \
}
And this function. Could it be that I misunderstood something?
int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
void __iomem *addr;
addr = bus->ops->map_bus(bus, devfn, where);
if (!addr)
return PCIBIOS_DEVICE_NOT_FOUND;
if (size == 1)
*val = readb(addr);
else if (size == 2)
*val = readw(addr);
else
*val = readl(addr);
return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(pci_generic_config_read);
> Instead I spent some more cycles on Lukas' and Mani's question about
> the value of the pci_bus_read_config() helper. So I changed
> PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions
> of read_cfg accessor functions like this:
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ac954584d991..9e2f75ede95f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
> ({
> \
> int __ttl = PCI_FIND_CAP_TTL;
> \
> u8 __id, __found_pos = 0;
> \
> - u32 __pos = (start);
> \
> - u32 __ent;
> \
> + u8 __pos = (start);
> \
> + u16 __ent;
> \
>
> \
> - read_cfg(args, __pos, 1, &__pos);
> \
> + read_cfg##_byte(args, __pos, &__pos);
> \
>
> \
> while (__ttl--) {
> \
> if (__pos < PCI_STD_HEADER_SIZEOF)
> \
> break;
> \
>
> \
> __pos = ALIGN_DOWN(__pos, 4);
> \
> - read_cfg(args, __pos, 2, &__ent);
> \
> + read_cfg##_word(args, __pos, &__ent);
> \
>
> \
> __id = FIELD_GET(PCI_CAP_ID_MASK, __ent);
> \
> if (__id == 0xff)
> \
> @@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>
> \
> __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> \
> while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {
> \
> - __ret = read_cfg(args, __pos, 4, &__header);
> \
> + __ret = read_cfg##_dword(args, __pos, &__header);
> \
> if (__ret != PCIBIOS_SUCCESSFUL)
> \
> break;
> \
>
> \
>
>
> This fixes the issue for s390's use-cases. With that
> pci_bus_read_config() becomes unused - and could be removed in further
> refinements.
>
> However, this probably breaks your dwc and cdns use-cases. I think,
> with the accessor functions for dwc and cadence changed to follow the
> {_byte|_word|_dword} naming pattern they could be used straight out of
> PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and
> cdns_pcie_read_cfg become obsolete as well.
>
> Thoughts?
In my opinion, it's no problem. I can provide the corresponding function
of {_byte / _word / _dword}. But it is necessary to know Bjorn, Mani,
Arnd, Lukas... Their viewpoints or suggestions.
Best regards,
Hans
Powered by blists - more mailing lists