linux-kernel - Re: [PATCH] PCI: Fix endianness issues in pci_bus_read

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: <ofsbfhor5ah3yzvkc5g5kb4fpjlzoqkkzukctmr3f6ur4vl2e7@7zvudt63ucbk>
Date: Fri, 1 Aug 2025 15:17:09 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Hans Zhang <hans.zhang@...tech.com>
Cc: Arnd Bergmann <arnd@...nel.org>, Bjorn Helgaas <helgaas@...nel.org>, 
	Gerd Bayer <gbayer@...ux.ibm.com>, Hans Zhang <18255117159@....com>, 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 Fri, Aug 01, 2025 at 05:25:51PM GMT, Hans Zhang wrote:
> 
> 
> On 2025/8/1 16:18, Manivannan Sadhasivam wrote:
> > EXTERNAL EMAIL
> > 
> > On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
> > > On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
> > > > On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> > > > > 
> > > > > -  if (size == 1)
> > > > > -          return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> > > > > -  else if (size == 2)
> > > > > -          return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> > > > > -  else if (size == 4)
> > > > > -          return pci_bus_read_config_dword(bus, devfn, where, val);
> > > > > -  else
> > > > > -          return PCIBIOS_BAD_REGISTER_NUMBER;
> > > > > +  if (size == 1) {
> > > > > +          rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> > > > > +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> > > > > +          *val = ((*val >> 24) & 0xff);
> > > > > +#endif
> > > > 
> > > > Yeah, this is all pretty ugly.  Obviously the previous code in
> > > > __pci_find_next_cap_ttl() didn't need this.  My guess is that was
> > > > because the destination for the read data was always the correct type
> > > > (u8/u16/u32), but here we always use a u32 and cast it to the
> > > > appropriate type.  Maybe we can use the correct types here instead of
> > > > the casts?
> > > 
> > > Agreed, the casts here just add more potential for bugs.
> > > 
> > 
> > Ack. Missed the obvious issue during review.
> > 
> > > 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.
> 
> 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.
> 
> 
> If there are no issues, am I sending the new version? Can this series of
> pacth 0001 be directly replaced?
> 

What benefit does this helper provide if it simply invokes the accessors based
on the requested size? IMO, the API should not return 'int' sized value if the
caller has explicitly requested to read variable size from config space.

- Mani

> 
> 
> 
> The patch is as follows:
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba66f55d2524..2bfd8fc1c0f5 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -89,15 +89,25 @@ int pci_bus_read_config(void *priv, unsigned int devfn,
> int where, u32 size,
>                         u32 *val)
>  {
>         struct pci_bus *bus = priv;
> +       int rc;
> +
> +       if (size == 1) {
> +               u8 byte;
> +
> +               rc = pci_bus_read_config_byte(bus, devfn, where, &byte);
> +               *val = byte;
> +       } else if (size == 2) {
> +               u16 word;
> +
> +               rc = pci_bus_read_config_word(bus, devfn, where, &word);
> +               *val = word;
> +       } else if (size == 4) {
> +               rc = pci_bus_read_config_dword(bus, devfn, where, val);
> +       } else {
> +               rc = PCIBIOS_BAD_REGISTER_NUMBER;
> +       }
> 
> -       if (size == 1)
> -               return pci_bus_read_config_byte(bus, devfn, where, (u8
> *)val);
> -       else if (size == 2)
> -               return pci_bus_read_config_word(bus, devfn, where, (u16
> *)val);
> -       else if (size == 4)
> -               return pci_bus_read_config_dword(bus, devfn, where, val);
> -       else
> -               return PCIBIOS_BAD_REGISTER_NUMBER;
> +       return rc;
>  }
> 
>  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> 
> 
> 
> Best regards,
> Hans
> 
> 
> 

-- 
மணிவண்ணன் சதாசிவம்