linux-kernel - Re: [PATCH v3 0/4] riscv: uaccess: optimizations

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

Message-ID: <4f6c83c0-39f6-467d-83c6-13d37440fb20@ghiti.fr>
Date: Fri, 14 Mar 2025 14:28:38 +0100
From: Alexandre Ghiti <alex@...ti.fr>
To: Cyril Bur <cyrilbur@...storrent.com>, palmer@...belt.com,
 aou@...s.berkeley.edu, paul.walmsley@...ive.com, charlie@...osinc.com,
 jrtc27@...c27.com, ben.dooks@...ethink.co.uk
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 jszhang@...nel.org
Subject: Re: [PATCH v3 0/4] riscv: uaccess: optimizations

Hi Cyril,

On 21/02/2025 01:09, Cyril Bur wrote:
> This series tries to optimize riscv uaccess by allowing the use of
> user_access_begin() and user_access_end() which permits grouping user accesses
> and avoiding the CSR write penalty for each access.
>
> The error path can also be optimised using asm goto which patches 3 and 4
> achieve. This will speed up jumping to labels by avoiding the need of an
> intermediary error type variable within the uaccess macros
>
> I did read the discussion this series generated. It isn't clear to me
> which direction to take the patches, if any.
>
> V2:
> I've taken on this series as there isn't any response from Jisheng. No
> significant changes other than build fixes.
> - Fixes build breakage in patch 3 to do with not having used 'goto' keyword.
> - Fixes build breakage in patch 4 on 32bit not having delcared __ptr in the
>    macro.
>
> V3:
> Significant commit message rewrites.
>   - Corrected the justification for patch 2
>   - Better explained/justified patches 3 and 4
> Minor code changes for legibility and more comments.
>
> Jisheng Zhang (4):
>    riscv: implement user_access_begin() and families
>    riscv: uaccess: use input constraints for ptr of __put_user()
>    riscv: uaccess: use 'asm goto' for put_user()
>    riscv: uaccess: use 'asm_goto_output' for get_user()
>
>   arch/riscv/include/asm/uaccess.h | 205 +++++++++++++++++++++++--------
>   1 file changed, 152 insertions(+), 53 deletions(-)
>
Following up on Ben's comment here 
https://lore.kernel.org/linux-riscv/b45aab1e-6d37-4027-9a15-4fa917d806b9@codethink.co.uk/

The problem that Ben mentions is caused by the use of *macros* which 
used to make the evaluation of the parameter inside the user-accessible 
section, and since this parameter could be a sleeping function, we could 
schedule another process with the SUM bit set, which could be cleared by 
this process, which would make the first process fault when trying to 
access user memory. I did not find any macro using unsafe_XXX() 
functions which could cause a problem right now, but I may have missed 
one and new could come up later, so we have multiple solutions here:

- suppose that a macro using unsafe_get/put_user() and passing a 
sleeping function as argument won't happen and then do nothing
- or save/restore CSR sstatus when switching processes
- or simply check that SUM is not set when switching processes

Let me know what you think.

Thanks,

Alex