linux-kernel - Re: [PATCH] asm-generic/io.h: Silence -Wnull-pointer-arithmetic warning on PCI

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: <CAK8P3a2NR2nhEffFQJdMq2Do_g2ji-7p3+iWyzw+aXD6gov05w@mail.gmail.com>
Date:   Tue, 13 Apr 2021 14:57:39 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Niklas Schnelle <schnelle@...ux.ibm.com>
Cc:     Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-s390 <linux-s390@...r.kernel.org>
Subject: Re: [PATCH] asm-generic/io.h: Silence -Wnull-pointer-arithmetic
 warning on PCI_IOBASE

On Tue, Apr 13, 2021 at 2:38 PM Niklas Schnelle <schnelle@...ux.ibm.com> wrote:
> On Tue, 2021-04-13 at 14:26 +0200, Arnd Bergmann wrote:
> > I think the real underlying problem is that '0' is a particularly bad
> > default value,
> > we should not have used this one in asm-generic, or maybe have left it as
> > mandatory to be defined by an architecture to a sane value. Note that
> > architectures that don't actually support I/O ports will cause a NULL
> > pointer dereference when loading a legacy driver, which is exactly what clang
> > is warning about here. Architectures that to support I/O ports in PCI typically
> > map them at a fixed location in the virtual address space and should put that
> > location here, rather than adding the pointer value to the port resources.
> >
> > What we might do instead here would be move the definition into those
> > architectures that actually define the base to be at address zero, with a
> > comment explaining why this is a bad location, and then changing the
> > inb/outb style helpers to either an empty function or a WARN_ONCE().
> >
> > On which architectures do you see the problem? How is the I/O port
> > actually mapped there?
>
> I'm seeing this on s390 which indeed has no I/O port support at all.
> I'm not sure how many others there are but I feel like us having to
> define these functions as empty is also kind of awkward. Maybe we could
> put them into the asm-generic/io.h for the case that PCI_IOBASE is not
> defined? Then at least every platform not supporting I/O ports would
> share them.

Yes, I meant doing this in the asm-generic version, something like

#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");
        return 0xff;
#endif
}
#endif

And follow up with a separate patch that moves the
"#define PCI_IOBASE ((void __iomem *)0)" into the architectures
that would currently see it, i.e. those that include asm-generic/io.h
but define neither inb/_inb nor PCI_IOBASE:

$ git grep -l asm-generic/io.h | xargs grep -wL inb | xargs grep -L PCI_IOBASE
arch/arc/include/asm/io.h
arch/csky/include/asm/io.h
arch/h8300/include/asm/io.h
arch/m68k/include/asm/io.h
arch/nds32/include/asm/io.h
arch/nios2/include/asm/io.h
arch/openrisc/include/asm/io.h
arch/s390/include/asm/io.h
arch/sparc/include/asm/io_32.h
arch/um/include/asm/io.h

Out of these, I see that arc, h8300, nds32, nios2, openrisc, and um
never set CONFIG_HAVE_PCI, so these would all be better off
leaving PCI_IOBASE undefined and getting the WARN_ONCE.

The remaining ones (csky, m68k, sparc32) need to be inspected
manually to see if they currently support PCI I/O space but in
fact use address zero as the base (with large resources) or they
should also turn the operations into a NOP.

         Arnd