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

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: <acd2e53f-b5c1-49c5-86e2-bc09eb917163@app.fastmail.com>
Date: Tue, 25 Jun 2024 07:54:30 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Jisheng Zhang" <jszhang@...nel.org>,
 "Paul Walmsley" <paul.walmsley@...ive.com>,
 "Palmer Dabbelt" <palmer@...belt.com>, "Albert Ou" <aou@...s.berkeley.edu>
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

On Tue, Jun 25, 2024, at 06:04, Jisheng Zhang wrote:
> 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.
>
> Signed-off-by: Jisheng Zhang <jszhang@...nel.org>

I think this is a bit too confusing: clearly there is no
read access from the __user pointer, so what you add in here
is not correct. There also needs to be a code comment about
why you do it this way, as it's not clear that this is
a workaround for old compilers without
CONFIG_CC_HAS_ASM_GOTO_OUTPUT.

> 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)
> 

I suspect this could just be a "r" constraint instead of
"m", treating the __user pointer as a plain integer.

For kernel pointers, using "m" and "=m" constraints
correctly is necessary since gcc will often access the
same data from C code as well. For __user pointers, we
can probably get away without it since no C code is
ever allowed to just dereference them. If you do that,
you may want to have the same thing in the __get_user
side.

      Arnd