linux-kernel - Re: [PATCH] riscv: misaligned: disable pagefault before accessing user memory

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: <af6dde21-39de-4142-acd6-0f2db931b257@rivosinc.com>
Date: Tue, 7 Jan 2025 09:32:29 +0100
From: Clément Léger <cleger@...osinc.com>
To: "Maciej W. Rozycki" <macro@...am.me.uk>
Cc: Alexandre Ghiti <alex@...ti.fr>, Paul Walmsley
 <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
 "open list:RISC-V ARCHITECTURE" <linux-riscv@...ts.infradead.org>,
 open list <linux-kernel@...r.kernel.org>,
 Samuel Holland <samuel.holland@...ive.com>
Subject: Re: [PATCH] riscv: misaligned: disable pagefault before accessing
 user memory

On 07/01/2025 03:33, Maciej W. Rozycki wrote:
> On Mon, 6 Jan 2025, Clément Léger wrote:
> 
>>> I'm wondering why the irqs are disabled here? What prevents us from
>>> enabling them? IIUC, if for some reason the user page is not in memory,
>>> we will fail the misaligned access and kill the process, so such
>>> failures could be completely random right?
>>
>> Hi Alex,
>>
>> This code is called from interrupt context as a result from an
>> exception. AFAIK, interrupts are disabled during the whole exception
>> handling and re-enabled when returning to user mode in
>> do_notify_resume(). So that's why it triggers this warning.
> 
>  This feels outright wrong.  We have analogous emulation code in Alpha and 
> MIPS ports and we stay away from quick sands by enabling interrupts in the 
> relevant exception handlers as soon as it is safe to do so; in particular 
> when emulating user-mode unaligned accesses.  What causes the RISC-V port 
> to be different here?

Hi Maciej,

I probably did not clarify that enough. The current path for misaligned
accesses does not re-enable irqs (whatever the mode we enter from). Some
other path does (page_fault) as it's indeed safe.

> 
>  See e.g. the MIPS exception dispatcher in arch/mips/kernel/genex.S where 
> BUILD_HANDLER invocations explicitly enable interrupts for most of the 
> handlers (via `sti' -> `__build_clear_sti').  Then unaligned emulation is 
> run with the interrupt enable setting preserved from the context the fault 
> happened in (via `ade' -> `__build_clear_ade'), clearly because it can in 
> principle trigger from kernel code run with interrupts disabled.  But such 
> code is not supposed to access user memory in the first place and traps 
> from the user mode will have had interrupts enabled.
> 
>  I think the RISC-V port has to do the same, i.e. run the handler with the 
> supervisor interrupt enable bit set according to whether interrupts were 
> enabled or not for the mode the fault has triggered in.

It seems like as you say, it would actually be safe to re-enable
interrupts in the misaligned load/store path as well as
do_trap()/do_trap_break() which might also benefit from this improvement.

After reviewing, the code, there is also a bunch of irqentry_nmi_enter()
that seems a bit weird, maybe they could be replaced with irqentry_enter().

Thanks for the review,

Clément

> 
>   Maciej