From: Arnd Bergmann <[email protected]>
Calling inb()/outb() on powerpc when CONFIG_PCI is disabled causes
a NULL pointer dereference, which is bad for a number of reasons.
After my patch to turn on -Werror in linux-next, this caused a
compiler-time warning with clang:
In file included from arch/powerpc/include/asm/io.h:672:
arch/powerpc/include/asm/io-defs.h:43:1: error: performing pointer
arithmetic on a null pointer has undefined behavior
[-Werror,-Wnull-pointer-arithmetic]
43 | DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
44 | (p, b, c), pio, p)
| ~~~~~~~~~~~~~~~~~~
In this configuration, CONFIG_HAS_IOPORT is already disabled, and all
drivers that use inb()/outb() should now depend on that (some patches are
still in the process of getting marged).
Hide all references to inb()/outb() in the powerpc code and the definitions
when HAS_IOPORT is disabled to remove the possible NULL pointer access.
The same should happin in asm-generic in the near future, but for now
the empty inb() macros are still defined to ensure the generic version
does not get pulled in.
Signed-off-by: Arnd Bergmann <[email protected]>
Reported-by: Naresh Kamboju <[email protected]>
--
Cc: [email protected]>
Cc: linuxppc-dev <[email protected]>
Cc: Aneesh Kumar K.V <[email protected]>
Cc: Anders Roxell <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Niklas Schnelle <[email protected]>
Cc: clang-built-linux <[email protected]>
Cc: Nick Desaulniers <[email protected]>
Cc: Nathan Chancellor <[email protected]>
Cc: Jeff Xu <[email protected]>
Cc: Naveen N. Rao <[email protected]>
Cc: Dan Carpenter <[email protected]>
---
arch/powerpc/include/asm/dma.h | 12 ++++++++----
arch/powerpc/include/asm/io-defs.h | 4 ++++
arch/powerpc/include/asm/io.h | 19 +++++++++++++++----
arch/powerpc/kernel/iomap.c | 4 ++++
arch/powerpc/kernel/traps.c | 2 +-
5 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/dma.h b/arch/powerpc/include/asm/dma.h
index d97c66d9ae34..004a868f82c9 100644
--- a/arch/powerpc/include/asm/dma.h
+++ b/arch/powerpc/include/asm/dma.h
@@ -3,6 +3,12 @@
#define _ASM_POWERPC_DMA_H
#ifdef __KERNEL__
+/* The maximum address that we can perform a DMA transfer to on this platform */
+/* Doesn't really apply... */
+#define MAX_DMA_ADDRESS (~0UL)
+
+#ifdef CONFIG_HAS_IOPORT
+
/*
* Defines for using and allocating dma channels.
* Written by Hennus Bergman, 1992.
@@ -26,10 +32,6 @@
#define MAX_DMA_CHANNELS 8
#endif
-/* The maximum address that we can perform a DMA transfer to on this platform */
-/* Doesn't really apply... */
-#define MAX_DMA_ADDRESS (~0UL)
-
#ifdef HAVE_REALLY_SLOW_DMA_CONTROLLER
#define dma_outb outb_p
#else
@@ -340,5 +342,7 @@ extern int request_dma(unsigned int dmanr, const char *device_id);
/* release it again */
extern void free_dma(unsigned int dmanr);
+#endif
+
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_DMA_H */
diff --git a/arch/powerpc/include/asm/io-defs.h b/arch/powerpc/include/asm/io-defs.h
index faf8617cc574..8d2209af7759 100644
--- a/arch/powerpc/include/asm/io-defs.h
+++ b/arch/powerpc/include/asm/io-defs.h
@@ -20,12 +20,14 @@ DEF_PCI_AC_NORET(writeq, (u64 val, PCI_IO_ADDR addr), (val, addr), mem, addr)
DEF_PCI_AC_NORET(writeq_be, (u64 val, PCI_IO_ADDR addr), (val, addr), mem, addr)
#endif /* __powerpc64__ */
+#ifdef CONFIG_HAS_IOPORT
DEF_PCI_AC_RET(inb, u8, (unsigned long port), (port), pio, port)
DEF_PCI_AC_RET(inw, u16, (unsigned long port), (port), pio, port)
DEF_PCI_AC_RET(inl, u32, (unsigned long port), (port), pio, port)
DEF_PCI_AC_NORET(outb, (u8 val, unsigned long port), (val, port), pio, port)
DEF_PCI_AC_NORET(outw, (u16 val, unsigned long port), (val, port), pio, port)
DEF_PCI_AC_NORET(outl, (u32 val, unsigned long port), (val, port), pio, port)
+#endif
DEF_PCI_AC_NORET(readsb, (const PCI_IO_ADDR a, void *b, unsigned long c),
(a, b, c), mem, a)
@@ -40,6 +42,7 @@ DEF_PCI_AC_NORET(writesw, (PCI_IO_ADDR a, const void *b, unsigned long c),
DEF_PCI_AC_NORET(writesl, (PCI_IO_ADDR a, const void *b, unsigned long c),
(a, b, c), mem, a)
+#ifdef CONFIG_HAS_IOPORT
DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
(p, b, c), pio, p)
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
@@ -52,6 +55,7 @@ DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
(p, b, c), pio, p)
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
(p, b, c), pio, p)
+#endif
DEF_PCI_AC_NORET(memset_io, (PCI_IO_ADDR a, int c, unsigned long n),
(a, c, n), mem, a)
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 08c550ed49be..86c212fcbc0c 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -37,7 +37,6 @@ extern struct pci_dev *isa_bridge_pcidev;
* define properly based on the platform
*/
#ifndef CONFIG_PCI
-#define _IO_BASE 0
#define _ISA_MEM_BASE 0
#define PCI_DRAM_OFFSET 0
#elif defined(CONFIG_PPC32)
@@ -486,8 +485,7 @@ static inline u64 __raw_rm_readq(volatile void __iomem *paddr)
* to port it over
*/
-#ifdef CONFIG_PPC32
-
+#if defined(CONFIG_PPC32) && defined(CONFIG_HAS_IOPORT)
#define __do_in_asm(name, op) \
static inline unsigned int name(unsigned int port) \
{ \
@@ -534,7 +532,7 @@ __do_out_asm(_rec_outb, "stbx")
__do_out_asm(_rec_outw, "sthbrx")
__do_out_asm(_rec_outl, "stwbrx")
-#endif /* CONFIG_PPC32 */
+#endif /* CONFIG_PPC32 && CONFIG_HAS_IOPORT */
/* The "__do_*" operations below provide the actual "base" implementation
* for each of the defined accessors. Some of them use the out_* functions
@@ -577,6 +575,7 @@ __do_out_asm(_rec_outl, "stwbrx")
#define __do_readq_be(addr) in_be64(PCI_FIX_ADDR(addr))
#endif /* !defined(CONFIG_EEH) */
+#ifdef CONFIG_HAS_IOPORT
#ifdef CONFIG_PPC32
#define __do_outb(val, port) _rec_outb(val, port)
#define __do_outw(val, port) _rec_outw(val, port)
@@ -592,6 +591,7 @@ __do_out_asm(_rec_outl, "stwbrx")
#define __do_inw(port) readw((PCI_IO_ADDR)_IO_BASE + port);
#define __do_inl(port) readl((PCI_IO_ADDR)_IO_BASE + port);
#endif /* !CONFIG_PPC32 */
+#endif
#ifdef CONFIG_EEH
#define __do_readsb(a, b, n) eeh_readsb(PCI_FIX_ADDR(a), (b), (n))
@@ -606,12 +606,14 @@ __do_out_asm(_rec_outl, "stwbrx")
#define __do_writesw(a, b, n) _outsw(PCI_FIX_ADDR(a),(b),(n))
#define __do_writesl(a, b, n) _outsl(PCI_FIX_ADDR(a),(b),(n))
+#ifdef CONFIG_HAS_IOPORT
#define __do_insb(p, b, n) readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
+#endif
#define __do_memset_io(addr, c, n) \
_memset_io(PCI_FIX_ADDR(addr), c, n)
@@ -689,6 +691,7 @@ static inline void name at \
#define writesb writesb
#define writesw writesw
#define writesl writesl
+
#define inb inb
#define inw inw
#define inl inl
@@ -848,8 +851,16 @@ static inline void iosync(void)
#define inl_p(port) inl(port)
#define outl_p(val, port) (udelay(1), outl((val), (port)))
+#define insb_p insb
+#define insw_p insw
+#define insl_p insl
+#define outsb_p outsb
+#define outsw_p outsw
+#define outsl_p outsl
+#ifdef CONFIG_HAS_IOPORT
#define IO_SPACE_LIMIT ~(0UL)
+#endif
/**
* ioremap - map bus memory into CPU space
diff --git a/arch/powerpc/kernel/iomap.c b/arch/powerpc/kernel/iomap.c
index 72862a4d3a5d..33e36fda1ea8 100644
--- a/arch/powerpc/kernel/iomap.c
+++ b/arch/powerpc/kernel/iomap.c
@@ -13,7 +13,11 @@
void __iomem *ioport_map(unsigned long port, unsigned int len)
{
+#ifdef CONFIG_HAS_IOPORT
return (void __iomem *) (port + _IO_BASE);
+#else
+ return NULL;
+#endif
}
EXPORT_SYMBOL(ioport_map);
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index f23430adb68a..b1c34242d394 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -531,7 +531,7 @@ DEFINE_INTERRUPT_HANDLER_NMI(system_reset_exception)
*/
static inline int check_io_access(struct pt_regs *regs)
{
-#ifdef CONFIG_PPC32
+#if defined(CONFIG_PPC32) && defined(CONFIG_HAS_IOPORT)
unsigned long msr = regs->msr;
const struct exception_table_entry *entry;
unsigned int *nip = (unsigned int *)regs->nip;
--
2.39.2
Arnd Bergmann <[email protected]> writes:
> From: Arnd Bergmann <[email protected]>
>
> Calling inb()/outb() on powerpc when CONFIG_PCI is disabled causes
> a NULL pointer dereference, which is bad for a number of reasons.
>
> After my patch to turn on -Werror in linux-next, this caused a
> compiler-time warning with clang:
>
> In file included from arch/powerpc/include/asm/io.h:672:
> arch/powerpc/include/asm/io-defs.h:43:1: error: performing pointer
> arithmetic on a null pointer has undefined behavior
> [-Werror,-Wnull-pointer-arithmetic]
> 43 | DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 44 | (p, b, c), pio, p)
> | ~~~~~~~~~~~~~~~~~~
>
> In this configuration, CONFIG_HAS_IOPORT is already disabled, and all
> drivers that use inb()/outb() should now depend on that (some patches are
> still in the process of getting marged).
>
> Hide all references to inb()/outb() in the powerpc code and the definitions
> when HAS_IOPORT is disabled to remove the possible NULL pointer access.
> The same should happin in asm-generic in the near future, but for now
> the empty inb() macros are still defined to ensure the generic version
> does not get pulled in.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
> Reported-by: Naresh Kamboju <[email protected]>
> --
This needs a small fixup:
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 86c212fcbc0c..60c80d0baf40 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -692,6 +692,7 @@ static inline void name at \
#define writesw writesw
#define writesl writesl
+#ifdef CONFIG_HAS_IOPORT
#define inb inb
#define inw inw
#define inl inl
@@ -704,6 +705,8 @@ static inline void name at \
#define outsb outsb
#define outsw outsw
#define outsl outsl
+#endif // CONFIG_HAS_IOPORT
+
#ifdef __powerpc64__
#define readq readq
#define writeq writeq
I'm running it through some randconfig builds now.
cheers
Michael Ellerman <[email protected]> writes:
> Arnd Bergmann <[email protected]> writes:
>> From: Arnd Bergmann <[email protected]>
>>
>> Calling inb()/outb() on powerpc when CONFIG_PCI is disabled causes
>> a NULL pointer dereference, which is bad for a number of reasons.
>>
>> After my patch to turn on -Werror in linux-next, this caused a
>> compiler-time warning with clang:
>>
>> In file included from arch/powerpc/include/asm/io.h:672:
>> arch/powerpc/include/asm/io-defs.h:43:1: error: performing pointer
>> arithmetic on a null pointer has undefined behavior
>> [-Werror,-Wnull-pointer-arithmetic]
>> 43 | DEF_PCI_AC_NORET(insb, (unsigned long p, void *b, unsigned long c),
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 44 | (p, b, c), pio, p)
>> | ~~~~~~~~~~~~~~~~~~
>>
>> In this configuration, CONFIG_HAS_IOPORT is already disabled, and all
>> drivers that use inb()/outb() should now depend on that (some patches are
>> still in the process of getting marged).
>>
>> Hide all references to inb()/outb() in the powerpc code and the definitions
>> when HAS_IOPORT is disabled to remove the possible NULL pointer access.
>> The same should happin in asm-generic in the near future, but for now
>> the empty inb() macros are still defined to ensure the generic version
>> does not get pulled in.
>>
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> Reported-by: Naresh Kamboju <[email protected]>
>> --
>
> This needs a small fixup:
Well, only because my tree doesn't have f0a816fb12da ("/dev/port: don't compile file operations without CONFIG_DEVPORT").
cheers
On Thu, Apr 18, 2024, at 08:26, Michael Ellerman wrote:
> Arnd Bergmann <[email protected]> writes:
> @@ -692,6 +692,7 @@ static inline void name at
> \
> #define writesw writesw
> #define writesl writesl
>
> +#ifdef CONFIG_HAS_IOPORT
> #define inb inb
> #define inw inw
> #define inl inl
> @@ -704,6 +705,8 @@ static inline void name at
> \
> #define outsb outsb
> #define outsw outsw
> #define outsl outsl
> +#endif // CONFIG_HAS_IOPORT
> +
> #ifdef __powerpc64__
> #define readq readq
> #define writeq writeq
I had included this at first, but then I still ran into
the same warnings because it ends up pulling in the
generic outsb() etc from include/asm-generic/io.h
that relies on setting a non-NULL PCI_IOBASE.
Arnd
"Arnd Bergmann" <[email protected]> writes:
> On Thu, Apr 18, 2024, at 08:26, Michael Ellerman wrote:
>> Arnd Bergmann <[email protected]> writes:
>
>> @@ -692,6 +692,7 @@ static inline void name at
>> \
>> #define writesw writesw
>> #define writesl writesl
>>
>> +#ifdef CONFIG_HAS_IOPORT
>> #define inb inb
>> #define inw inw
>> #define inl inl
>> @@ -704,6 +705,8 @@ static inline void name at
>> \
>> #define outsb outsb
>> #define outsw outsw
>> #define outsl outsl
>> +#endif // CONFIG_HAS_IOPORT
>> +
>> #ifdef __powerpc64__
>> #define readq readq
>> #define writeq writeq
>
> I had included this at first, but then I still ran into
> the same warnings because it ends up pulling in the
> generic outsb() etc from include/asm-generic/io.h
> that relies on setting a non-NULL PCI_IOBASE.
Yes you're right. The above fixes the gcc build, but not clang.
So I think I'll just cherry pick f0a816fb12da ("/dev/port: don't compile
file operations without CONFIG_DEVPORT") into my next and then apply
this. But will see if there's any other build failures over night.
cheers
Michael Ellerman <[email protected]> writes:
> "Arnd Bergmann" <[email protected]> writes:
>> On Thu, Apr 18, 2024, at 08:26, Michael Ellerman wrote:
>>> Arnd Bergmann <[email protected]> writes:
>>
>>> @@ -692,6 +692,7 @@ static inline void name at
>>> \
>>> #define writesw writesw
>>> #define writesl writesl
>>>
>>> +#ifdef CONFIG_HAS_IOPORT
>>> #define inb inb
>>> #define inw inw
>>> #define inl inl
>>> @@ -704,6 +705,8 @@ static inline void name at
>>> \
>>> #define outsb outsb
>>> #define outsw outsw
>>> #define outsl outsl
>>> +#endif // CONFIG_HAS_IOPORT
>>> +
>>> #ifdef __powerpc64__
>>> #define readq readq
>>> #define writeq writeq
>>
>> I had included this at first, but then I still ran into
>> the same warnings because it ends up pulling in the
>> generic outsb() etc from include/asm-generic/io.h
>> that relies on setting a non-NULL PCI_IOBASE.
>
> Yes you're right. The above fixes the gcc build, but not clang.
>
> So I think I'll just cherry pick f0a816fb12da ("/dev/port: don't compile
> file operations without CONFIG_DEVPORT") into my next and then apply
> this. But will see if there's any other build failures over night.
That didn't work. Still lots of drivers in my tree (based on rc2) which
use inb/outb etc, and barf on the empty #define inb.
So I think this patch needs to wait until all the CONFIG_HAS_IOPORT
checks have been merged for various drivers.
For now the below fixes the clang warning. AFAICS it's safe because any
code using inb() etc. with CONFIG_PCI=n is currently just doing a plain
load from virtual address ~zero which should fault anyway.
cheers
diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
index 08c550ed49be..1cd6eb6c8101 100644
--- a/arch/powerpc/include/asm/io.h
+++ b/arch/powerpc/include/asm/io.h
@@ -37,7 +37,7 @@ extern struct pci_dev *isa_bridge_pcidev;
* define properly based on the platform
*/
#ifndef CONFIG_PCI
-#define _IO_BASE 0
+#define _IO_BASE POISON_POINTER_DELTA
#define _ISA_MEM_BASE 0
#define PCI_DRAM_OFFSET 0
#elif defined(CONFIG_PPC32)
On Fri, Apr 19, 2024, at 07:12, Michael Ellerman wrote:
> Michael Ellerman <[email protected]> writes:
>> "Arnd Bergmann" <[email protected]> writes:
>>>
>>> I had included this at first, but then I still ran into
>>> the same warnings because it ends up pulling in the
>>> generic outsb() etc from include/asm-generic/io.h
>>> that relies on setting a non-NULL PCI_IOBASE.
>>
>> Yes you're right. The above fixes the gcc build, but not clang.
>>
>> So I think I'll just cherry pick f0a816fb12da ("/dev/port: don't compile
>> file operations without CONFIG_DEVPORT") into my next and then apply
>> this. But will see if there's any other build failures over night.
>
> That didn't work. Still lots of drivers in my tree (based on rc2) which
> use inb/outb etc, and barf on the empty #define inb.
Right, the patches from Niklas only went into linux-next so far,
and a few are missing (including the 8250 one I think), so -rc2
at the moment regresses, but that doesn't have the warning either.
The idea of my patch was to both fix the current linux-next
build regression and have something that works in the long
run, I didn't expect it to work by itself. Sorry that wasn't
clear from my description.
> So I think this patch needs to wait until all the CONFIG_HAS_IOPORT
> checks have been merged for various drivers.
>
> For now the below fixes the clang warning. AFAICS it's safe because any
> code using inb() etc. with CONFIG_PCI=n is currently just doing a plain
> load from virtual address ~zero which should fault anyway.
If the port number is high enough, the current code might end
up referencing a user space address, depending on mmap_min_addr,
which defaults to 4096.
Using POISON_POINTER_DELTA is clearly an improvement over that.
> diff --git a/arch/powerpc/include/asm/io.h b/arch/powerpc/include/asm/io.h
> index 08c550ed49be..1cd6eb6c8101 100644
> --- a/arch/powerpc/include/asm/io.h
> +++ b/arch/powerpc/include/asm/io.h
> @@ -37,7 +37,7 @@ extern struct pci_dev *isa_bridge_pcidev;
> * define properly based on the platform
> */
> #ifndef CONFIG_PCI
> -#define _IO_BASE 0
> +#define _IO_BASE POISON_POINTER_DELTA
> #define _ISA_MEM_BASE 0
> #define PCI_DRAM_OFFSET 0
> #elif defined(CONFIG_PPC32)
You may need to double-check, but I think for ppc64 we
can just unconditionally set _IO_BASE to ISA_IO_BASE
regardless of CONFIG_PCI. 3d5134ee8341 ("[POWERPC]
Rewrite IO allocation & mapping on powerpc64") ensured
that the I/O space is only ever mapped to this virtual
address, and the same method is used with the
asm-generic/io.h implementation on arm/arm64/loongarch/
m68k/riscv/xtensa. Using this would be both safer and
more efficient than the current version.
It should also not cause any regressions ;-)
Unfortunately, ppc32 never got that cleanup, so
POISON_POINTER_DELTA is probably still best until Niklas's
series is merged. You could set _ISA_MEM_BASE to the same
here for good measure.
[another side note: the non-zero PCI_DRAM_OFFSET looks
unnecessary as well now, apparently this was meant for
ibm cpc710 and ppc440 platforms that are no longer
supported.]
Arnd