linux-kernel - Re: [PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of __put_user

Open Source and information security mailing list archives

Message-Id: <EA2CB85E-E210-402E-B891-2F4420E55CDB@jrtc27.com>
Date: Fri, 17 Jan 2025 23:34:49 +0000
From: Jessica Clarke <jrtc27@...c27.com>
To: Cyril Bur <cyrilbur@...storrent.com>
Cc: palmer@...belt.com,
 aou@...s.berkeley.edu,
 paul.walmsley@...ive.com,
 linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org,
 Jisheng Zhang <jszhang@...nel.org>
Subject: Re: [PATCH v2 2/4] riscv: uaccess: use input constraints for ptr of
 __put_user

On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@...storrent.com> wrote:
> 
> From: Jisheng Zhang <jszhang@...nel.org>
> 
> I believe the output constraints "=m" is not necessary, because
> the instruction itself is "write", we don't need the compiler
> to "write" for us. So tell compiler we read from memory instead
> of writing.

That’s not what =m does. =m tells the compiler “this assembly will be
writing to this memory location, and needs the address of the location
for that operand”. If you move it from an output to an input then you
get the same assembly generated, but the compiler believes you are
*reading* from that memory, not *writing* to it, and so does not
believe the memory may be clobbered.

Now, it may well be that, by virtue of this being userspace memory that
is only ever accessed via assembly, and any inline assembly being
marked volatile, this in effect doesn’t matter. But it is still
technically wrong to model it that way, and you cannot use the
justification you are using, because it is false, and demonstrates a
lack of understanding for how inline assembly works. It has to be
correctly justified.

(I do note that neither x86 nor arm64 seems to model this as an output)

Jess

> Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> Signed-off-by: Cyril Bur <cyrilbur@...storrent.com>
> ---
> arch/riscv/include/asm/uaccess.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 09d4ca37522c..84b084e388a7 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -186,11 +186,11 @@ do { \
> __typeof__(*(ptr)) __x = x; \
> __asm__ __volatile__ ( \
> "1:\n" \
> - " " insn " %z2, %1\n" \
> + " " insn " %z1, %2\n" \
> "2:\n" \
> _ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0) \
> - : "+r" (err), "=m" (*(ptr)) \
> - : "rJ" (__x)); \
> + : "+r" (err) \
> + : "rJ" (__x), "m"(*(ptr))); \
> } while (0)
> 
> #ifdef CONFIG_64BIT
> @@ -203,16 +203,16 @@ do { \
> u64 __x = (__typeof__((x)-(x)))(x); \
> __asm__ __volatile__ ( \
> "1:\n" \
> - " sw %z3, %1\n" \
> + " sw %z1, %3\n" \
> "2:\n" \
> - " sw %z4, %2\n" \
> + " sw %z2, %4\n" \
> "3:\n" \
> _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0) \
> _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0) \
> - : "+r" (err), \
> - "=m" (__ptr[__LSW]), \
> - "=m" (__ptr[__MSW]) \
> - : "rJ" (__x), "rJ" (__x >> 32)); \
> + : "+r" (err) \
> + : "rJ" (__x), "rJ" (__x >> 32), \
> + "m" (__ptr[__LSW]), \
> + "m" (__ptr[__MSW])); \
> } while (0)
> #endif /* CONFIG_64BIT */
> 
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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