linux-kernel - Re: how to fix acpi_pci_root_remap

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]

Date:   Fri, 17 Aug 2018 10:55:51 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Tony Luck <tony.luck@...el.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        jchandra@...adcom.com, Sinan Kaya <okaya@...eaurora.org>,
        Tomasz Nowicki <tn@...ihalf.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Miquel Raynal <miquel.raynal@...tlin.com>
Subject: Re: how to fix acpi_pci_root_remap_iospace?

On Fri, 17 Aug 2018 10:47:34 +0200
Arnd Bergmann <arnd@...db.de> wrote:

> On Fri, Aug 17, 2018 at 1:27 AM Luck, Tony <tony.luck@...el.com> wrote:
> >
> > On Thu, Aug 16, 2018 at 11:10:33PM +0200, Arnd Bergmann wrote:  
> > > Another way would be to add
> > >
> > >  #include <asm-generic/io.h>
> > > +#undef PCI_IOBASE
> > >
> > > in your asm/io.h. This is about as ugly as the your version, but
> > > it would be local to ia64 ;-)  
> >
> > Third way ...
> >
> >
> > Is "0" actually the right value for PCI_IOBASE for some platform?
> >
> > #ifndef PCI_IOBASE
> > #define PCI_IOBASE ((void __iomem *)0)
> > #endif
> >
> > Or is this just here to make sure that:
> >
> > static inline u8 inb(unsigned long addr)
> > {
> >         u8 val;
> >
> >         __io_pbr();
> >         val = __raw_readb(PCI_IOBASE + addr);
> >         __io_par();
> >         return val;
> > }
> >
> > etc. Do not throw errors?  
> 
> Defining it to zero is the traditional approach on some systems, and it's used
> for at least two different reasons, both of which I don't particularly like:
> 
> - Some (particularly older) targets that have its I/O space mapped
> into its linear
>   virtual memory define inb() to be effectively an alias for readb() with the
>   same numeric arguments. This kind of works in most cases but breaks in
>   many corner cases such as
>   * user space using /dev/ioport, which now grants access to all of
> kernel memory
>   * ISA device drivers using fixed 16-bit addresses on inb/outb, which
> now points
>     into user space memory
>   * drivers that get the correct address from a resource but then truncate it by
>     storing it in a 16-bit or 32-bit (on 64-bit machines) local variable.
> 
> - Some targets don't have any support for I/O space on their PCI bus and just
>    want to get things to compile by setting PCI_IOBASE to zero, this still opens
>     up some of the same problems as above, but doesn't really help otherwise.
> 
> > Should we really just enclose all of inb, inw, inl, ...
> > inside of:
> >
> > #ifdef PCI_IOBASE
> >
> > ... all those static functions that use PCI_IOBASE ...  
> 
> This breaks compilation of a couple of important drivers such as serial-8250
> which support either I/O or memory space, so it requires some cleanup
> first, or the definition of an alternative nop inb/outb family that does not
> try to access the bus.

Hm, maybe it's just easier to revert the patch since we got rid of
patches adding COMPILE_TEST to drivers which were using read/writesl()
(it turned out ia64 and sparc were not the only archs to not implement
readsx/writesx() variants, and fixing them is not that easy).