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

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

[Michael Ellerman]

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
 #define _ISA_MEM_BASE	0
 #define PCI_DRAM_OFFSET 0
 #elif defined(CONFIG_PPC32)
-- 
2.44.0

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

[Nathan Chancellor]

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
> 

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

[Michael Ellerman]

Nathan Chancellor <nathan@kernel.org> writes:
> Hi Michael,
>
> On Wed, May 01, 2024 at 12:04:40AM +1000, Michael Ellerman wrote:
...
>> 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

:facepalm:

Looks like we can fix the compiler warnings just by doing the arithmetic
before casting to void *. I'll send a v2.

cheers