linux-kernel - Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access

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: <CAK8P3a1D5DzmNGsEPQomkyMCmMrtD6pQ11JRMh78vbY53edp-Q@mail.gmail.com>
Date:   Sat, 3 Jul 2021 14:12:26 +0200
From:   Arnd Bergmann <arnd@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-arch <linux-arch@...r.kernel.org>,
        linux-pci <linux-pci@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Niklas Schnelle <schnelle@...ux.ibm.com>
Subject: Re: [GIT PULL 1/2] asm-generic: rework PCI I/O space access

On Fri, Jul 2, 2021 at 9:42 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Fri, Jul 2, 2021 at 6:48 AM Arnd Bergmann <arnd@...nel.org> wrote:
> >
> > A rework for PCI I/O space access from Niklas Schnelle:
>
> I pulled this, but then I ended up unpulling.
>
> I don't absolutely _hate_ the concept, but I really find this to be
> very unpalatable:
>
>   #if !defined(inb) && !defined(_inb)
>   #define _inb _inb
>   static inline u8 _inb(unsigned long addr)
>   {
>   #ifdef PCI_IOBASE
>         u8 val;
>
>         __io_pbr();
>         val = __raw_readb(PCI_IOBASE + addr);
>         __io_par(val);
>         return val;
>   #else
>         WARN_ONCE(1, "No I/O port support\n");
>         return ~0;
>   #endif
>   }
>   #endif
>
> because honestly, the notion of a run-time warning for a compile-time
> "this cannot work" is just wrong.

Ok, fair enough, back to the drawing board then.

> If the platform doesn't have inb/outb, and you compile some driver
> that uses them, you don't want a run-time warning. Particularly since
> in many cases nobody will ever run it, and the main use case was to do
> compile-testing across a wide number of platforms.
>
> So if the platform doesn't have inb/outb, they simply should not be
> declared, and there should be a *compile-time* error. That is
> literally a lot more useful, and it avoids this extra code.

I tried adding a Kconfig option over a decade ago, but at the time
gave up when I couldn't still get drivers/ide and the 8250 uart driver
to build in a sensible way that would still allow the MMIO based
variants to work, but leave out the PIO accessors. With drivers/ide
gone, and the drivers/tty/serial/ having gone through many changes,
it's probably easier now.

I could imagine adding a CONFIG_LEGACY_PCI that controls
whether we have any pre-PCIe devices or those PCIe drivers
that need PIO accessors other than ioport_map()/pci_iomap().

This can then select a CONFIG_IOPORT, which controls whether
inb/outb etc are provided. x86 and anything that uses inb/outb for
non-PCI devices would select it as well.

> Extra code that not only doesn't add value, but that actually
> *subtracts* value is not code I really want to pull.

What happened here specifically is that the asm-generic version
is definitely broken and can cause a NULL pointer dereference
on platforms that used to fall back to NULL PCI_IOBASE.

The latest clang does complain about those drivers with a
correct warning (not an error) that shows up in s390 allmodconfig
builds. Niklas' original version of the patch tried to shut up the
warning but did not address the dangerous behavior, which I
did not find sufficient either.

The version we got here makes it no longer crash the kernel, but
I see your point that the runtime warning is still wrong. I'll have
a look at what it would take to guard all inb/outb callers with a
Kconfig conditional, and will report back after that.

      Arnd