Re: [PATCH] powerpc: Set _IO_BASE to POISON_POINTER_DELTA not 0 for CONFIG_PCI=n

[Nathan Chancellor <nathan@kernel.org>]

Hi Michael,

On Wed, May 01, 2024 at 12:04:40AM +1000, Michael Ellerman wrote:
> With -Wextra clang warns about pointer arithmetic using a null pointer.
> When building with CONFIG_PCI=n, that triggers a warning in the IO
> accessors, eg:
> 
>   In file included from linux/arch/powerpc/include/asm/io.h:672:
>   linux/arch/powerpc/include/asm/io-defs.h:23:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
>      23 | DEF_PCI_AC_RET(inb, u8, (unsigned long port), (port), pio, port)
>         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ...
>   linux/arch/powerpc/include/asm/io.h:591:53: note: expanded from macro '__do_inb'
>     591 | #define __do_inb(port)          readb((PCI_IO_ADDR)_IO_BASE + port);
>         |                                       ~~~~~~~~~~~~~~~~~~~~~ ^
> 
> That is because when CONFIG_PCI=n, _IO_BASE is defined as 0.
> 
> There is code that builds with calls to IO accessors even when
> CONFIG_PCI=n, but the actual calls are guarded by runtime checks.
> If not those calls would be faulting, because the page at virtual
> address zero is (usually) not mapped into the kernel. As Arnd pointed
> out, it is possible a large port value could cause the address to be
> above mmap_min_addr which would then access userspace, which would be
> a bug.
> 
> To avoid any such issues, and also fix the compiler warning, set the
> _IO_BASE to POISON_POINTER_DELTA. That is a value chosen to point into
> unmapped space between the kernel and userspace, so any access will
> always fault.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Closes: https://lore.kernel.org/all/CA+G9fYtEh8zmq8k8wE-8RZwW-Qr927RLTn+KqGnq1F=ptaaNsA@mail.gmail.com
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/io.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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

This works for CONFIG_PPC64 but not CONFIG_PPC32 (so tinyconfig and
allnoconfig like Naresh reported) because CONFIG_ILLEGAL_POINTER_VALUE
is defined as 0 in that case.

  $ grep -P 'CONFIG_(ILLEGAL_POINTER_VALUE|PCI|PPC32)' .config
  CONFIG_PPC32=y
  CONFIG_ILLEGAL_POINTER_VALUE=0

>  #define _ISA_MEM_BASE	0
>  #define PCI_DRAM_OFFSET 0
>  #elif defined(CONFIG_PPC32)
> -- 
> 2.44.0
>