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]

Date: Wed, 26 Jun 2024 16:25:26 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Jisheng Zhang" <jszhang@...nel.org>, "Andreas Schwab" <schwab@...e.de>
Cc: "Paul Walmsley" <paul.walmsley@...ive.com>,
 "Palmer Dabbelt" <palmer@...belt.com>, "Albert Ou" <aou@...s.berkeley.edu>,
 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 Wed, Jun 26, 2024, at 15:12, Jisheng Zhang wrote:
> On Wed, Jun 26, 2024 at 03:12:50PM +0200, Andreas Schwab wrote:
>> On Jun 25 2024, 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.
>> 
>> No, this is backwards.  Being an output operand means that the *asm* is
>> writing to it, and the compiler can read the value from there afterwards
>> (and the previous value is dead before the asm).
>
> Hi Andreas,
>
> I compared tens of __put_user() caller's generated code between orig
> version and patched version, they are the same. Sure maybe this is
> not enough. 
>
> But your explanation can be applied to x86 and arm64 __put_user()
> implementations, asm is also writing, then why there's no output
> constraints there?(see the other two emails)? Could you please help
> me to understand the tricky points?

I think part of the reason for the specific way the x86
user access is written is to work around bugs in old
compiler versions, as well as to take advantage of the
complex addressing modes in x86 assembler, see this bit
that dates back to the earliest version of the x86_64
codebase and is still left in place:

/* FIXME: this hack is definitely wrong -AK */
struct __large_struct { unsigned long buf[100]; };
#define __m(x) (*(struct __large_struct __user *)(x))

Using the memory input constraint means that x86 can use
a load from a pointer plus offset, but riscv doesn't
actually do this. The __large_struct I think was needed
either to prevent the compiler from reading the data
outside of the assembly, or to tell the compiler about
the fact that there is an actual memory access if
__put_user() was pointed at kernel memory.

If you just copy from the arm64 version that uses an
"r"(address) constraint instead of the "m"(*address)
version, it should be fine for any user space access.

The output constraint is technically still be needed
if we have code like this one where we actually write to
something in kernel space:

int f(void)
{
     int a = 1;
     int b = 2;
     __put_kernel_nofault(&a, &b, int, error);
     return a;
error:
     return -EFAULT;
}

In this case, __put_kernel_nofault() writes the value
of b into a, but the compiler can safely assume that
a is not changed by the assembly because there is no
memory output, and would likely just return a constant '1'. 

For put_user(), this cannot happen because the compiler
doesn't know anything about the contents of the __user
pointer. For __put_kernel_nofault(), we rely on the
callers never using it on pointers they access, which
is probably a reasonable assumption, but not entirely
correct.

     Arnd