linux-kernel - Re: [PATCH] RISC-V: Fix PCI I/O port addressing for MMU-less configurations

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] [day] [month] [year] [list]

Message-Id: <b2424c71-d076-4880-86cd-ccf74995f080@app.fastmail.com>
Date: Fri, 11 Apr 2025 07:41:33 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Liu Runrun" <liurunrun@...ontech.com>,
 "Paul Walmsley" <paul.walmsley@...ive.com>,
 "Palmer Dabbelt" <palmer@...belt.com>, "Albert Ou" <aou@...s.berkeley.edu>,
 "Alexandre Ghiti" <alex@...ti.fr>, "Ingo Molnar" <mingo@...nel.org>,
 "Ryan Roberts" <ryan.roberts@....com>,
 "Catalin Marinas" <catalin.marinas@....com>,
 "Andrew Morton" <akpm@...ux-foundation.org>,
 "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Linux-Arch <linux-arch@...r.kernel.org>, WangYuli <wangyuli@...ontech.com>,
 zhanjun@...ontech.com, niecheng1@...ontech.com
Subject: Re: [PATCH] RISC-V: Fix PCI I/O port addressing for MMU-less configurations

On Fri, Apr 11, 2025, at 04:34, Liu Runrun wrote:
> This patch addresses the PCI I/O port address handling in RISC-V's
> port-mapped I/O emulation routines when the MMU is not enabled.
> The changes ensure that:

Do you have a system that requires this? I sent a patch the other
day to make PCI 'depends on MMU', based on how nothing today
uses it. Having a NOMMU system with PCI wounds rather silly,
so I hope we don't ever get that.

> 1. For non-MMU systems, the PCI I/O port addresses are properly
>    calculated in marcos inX and outX when PCI_IOBASE is not
>    defined, this avoids the null pointer calculating warning
>    from the compiler.

This is the wrong way around: the warning tells you that you
have failed to configure PCI_IOBASE for the particular hardware,
and that you actually get a NULL pointer dereference.

The solution is not to shut up the warning but making it
not a NULL pointer dereference!

Part of the issue is that historically the asm-generic/io.h
header includes an incorrect fallback of PCI_IOBASE when
the architecture does not provide the correct one. I think only
sparc still relies on that, so that fallback definition should
be moved into arch/sparc/include/asm/io_{32,64}.h instead.

> 2. In asm-generic/io.h, function ioport_map(), casting PCI_IOPORT
>    to type "long" firstly makes it could compute with variable addr
>    directly, which avoids the null pointer calculating warning when
>    PCI_IOPORT is a null pointer in some case.

I don't understand that sentence, please rephrase.

> The original implementation used `PCI_IOBASE + (addr)` for MMU-enabled
> systems, but failed to handle non-MMU cases correctly. This change adds
> conditional compilation guards (#ifdef CONFIG_MMU) to differentiate
> between MMU and non-MMU environments, providing consistent behavior
> for both scenarios.

This also looks wrong: what you are distinguishing here is systems
with (potentially) I/O port support and those that never have I/O
port support.

> diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
> index a0e51840b9db..d5181bb02c98 100644
> --- a/arch/riscv/include/asm/io.h
> +++ b/arch/riscv/include/asm/io.h
> @@ -101,9 +101,15 @@ __io_reads_ins(reads, u32, l, __io_br(), 
> __io_ar(addr))
>  __io_reads_ins(ins,  u8, b, __io_pbr(), __io_par(addr))
>  __io_reads_ins(ins, u16, w, __io_pbr(), __io_par(addr))
>  __io_reads_ins(ins, u32, l, __io_pbr(), __io_par(addr))
> +#ifdef CONFIG_MMU
>  #define insb(addr, buffer, count) __insb(PCI_IOBASE + (addr), buffer, 
> count)
>  #define insw(addr, buffer, count) __insw(PCI_IOBASE + (addr), buffer, 
> count)
>  #define insl(addr, buffer, count) __insl(PCI_IOBASE + (addr), buffer, 
> count)

I see that these are defined unconditionally here, which is probably
the real mistake. What I think we need instead is to enclose
them in "#ifdef CONFIG_HAS_IOPORT" with no "#else" block, and
ensure that HAS_IOPORT is only set when building for targets that
have a sensible definition of PCI_IOBASE and IO_SPACE_LIMIT.

The same approach is used in asm-generic/io.h, we just haven't
done it for all architectures yet, since it's not entirely
clear which ones can support ISA bridges and legacy PCI devices
with port I/O.

Ideally we'd go through the individual PCI host driver
implementations and only enable HAS_IOPORT for those that
are can handle the I/O space window correctly, but leave
it out if none of them are selected.

      Arnd