linux-kernel - Re: [PATCH 1/1] tty: serial: handle HAS

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: <e916ff3347cef88981d8e519fe1bcedfabfbea24.camel@linux.ibm.com>
Date: Fri, 04 Oct 2024 12:09:18 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: "Maciej W. Rozycki" <macro@...am.me.uk>, Arnd Bergmann <arnd@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby
 <jirislaby@...nel.org>,
        Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>,
        linux-serial@...r.kernel.org, Heiko
 Carstens <hca@...ux.ibm.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

On Wed, 2024-10-02 at 23:59 +0100, Maciej W. Rozycki wrote:
> On Wed, 2 Oct 2024, Arnd Bergmann wrote:
> 
> > I agree that this shouldn't be hard to finish. The IS_ENABLED()
> > check is not that easy to do as I think we need to keep calling
> > inb()/outb() outside of an #ifdef a compile-time error.
> 
>  Can we just provide dummy prototypes with `__attribute__((error("...")))' 
> instead?  This will surely prevent us from having to bend backwards so as 
> to make sure the compiler won't see any spurious references to these 
> inexistent functions or macros.  We already have a `__compiletime_error()' 
> macro for this purpose even.

This is already done in the final patch of my series when disabling
inb()/outb() and friends.

> 
> > Part of the problem that Niklas is trying to solve with the
> > CONFIG_HAS_IOPORT annotations is to prevent an invalid inb()/outb()
> > from turning into a NULL pointer dereference as it currently does
> > on architectures that have no way to support PIO but get the
> > default implementation from asm-generic/io.h.
> 
>  It can be worse than that.  Part of my confusion with the defxx driver 
> trying to do port I/O with my POWER9 system came from the mapping actually 
> resulting in non-NULL invalid pointers, dereferencing which caused a flood 
> of obscure messages produced to the system console by the system firmware:
> 
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> LPC[000]: Got SYNC no-response error. Error address reg: 0xd0010014
> IPMI: dropping non severe PEL event
> [...]
> 
> from which it was all but obvious that they were caused by an attempt to 
> use PCI port I/O with a system lacking support for it.
> 
> > It's not clear if having a silently non-working driver or one
> > that crashes makes it easier to debug for users. Having a clear
> > warning message in the PCI probe code is probably the best
> > we can hope for.
> 
>  I agree.  Enthusiastically.

I think there was also a bit of a misunderstanding. My argument that
this would be very ugly in the general case was really meant as general
case outside of drivers like 8250 that deals with both I/O port and
MMIO i.e. we can't warn/error when !HAS_IOPORT deactivates a whole
driver because seeing an I/O port BAR in common PCI code doesn't mean
that it is required for use of the device.

I'm working on a new proposal for 8250 now. Basically I think we can
put the warning/error in serial8250_pci_setup_port(). And then for
those 8250_pci.c "sub drivers" that require I/O ports instead of just
ifdeffing out their setup/init/exit we can define anything but setup to
NULL and setup to pci_default_setup() such that the latter will find
the I/O port BAR via FL_GET_BASE() and subsequently cause the error
print in serial8250_pci_setup_port(). It's admittedly a bit odd but it
also keeps the #ifdefs to just around the code that wouldn't compile.

One thing I'm not happy with yet is the handling around
is_upf_fourport(port) in 8250_pci.c. With !HAS_IOPORT this is defined
as false. With that it makes sure that inb_p()/outb_p() aren't called
but I think this only works because the compiler usually drops the dead
if clause. I think there were previous discussions around this but I
think just like IS_ENABLED() checks this isn't quite correct.

Thanks,
Niklas