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: <ZnwObmA70Bfx9yCn@xhacker>
Date: Wed, 26 Jun 2024 20:49:50 +0800
From: Jisheng Zhang <jszhang@...nel.org>
To: Arnd Bergmann <arnd@...db.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 08:32:38PM +0800, Jisheng Zhang wrote:
> On Tue, Jun 25, 2024 at 07:54:30AM +0200, Arnd Bergmann wrote:
> > 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
> 
> Here is my understanding: the __put_user is implemented with
> sd(or its less wider variant, sw etc.), w/o considering the
> ex_table, the previous code can be simplified as below:
> 
> __asm__ __volatile__ (
> 	"sw	%z2, %1\n"
> 	: "+r" (err), "=m" (*(ptr))
> 	: "rJ" (__x));
> 
> Here ptr is really an input, just tells gcc where to store,
> And the "store" action is from the "sw" instruction, I don't
> need the gcc generates "store" instruction for me. so IMHO,
> there's no need to use output constraints here. so I changed
> it to
> 
> __asm__ __volatile__ (
> 	"sw	%z1, %2\n"
> 	: "+r" (err)
> 	: "rJ" (__x), "m"(*(ptr)));
> 
> The key here: is this correct?
> 
> 
> Here is the put_user piece code and comments from x86
> 
> /*
>  * Tell gcc we read from memory instead of writing: this is because
>  * we do not write to any memory gcc knows about, so there are no
>  * aliasing issues.
>  */
> #define __put_user_goto(x, addr, itype, ltype, label)                   \
>         asm goto("\n"                                                   \
>                 "1:     mov"itype" %0,%1\n"                             \
>                 _ASM_EXTABLE_UA(1b, %l2)                                \
>                 : : ltype(x), "m" (__m(addr))                           \
>                 : : label)

Here is the simplified put_user piece code of arm64:

#define __put_mem_asm(store, reg, x, addr, err, type)                   \
        asm volatile(                                                   \
        "1:     " store "       " reg "1, [%2]\n"                       \
        "2:\n"                                                          \
        _ASM_EXTABLE_##type##ACCESS_ERR(1b, 2b, %w0)                    \
        : "+r" (err)                                                    \
        : "rZ" (x), "r" (addr))

no output constraints either. It just uses "r" input constraints to tell
gcc to read the store address into one proper GP reg.

> 
> 
> As can be seen, x86 also doesn't put the (addr) in output constraints,
> I think x86 version did similar modification in history, but when I tried
> to searh the git history, the comment is there from the git first day.
> 
> Any hint or suggestion is appreciated!
> 
> > 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.
> 
> I tried "r", the generated code is not as good as "m"
> 
> for example
> __put_user(0x12, &frame->uc.uc_flags);
> 
> with "m", the generated code will be
> 
> ...
> csrs    sstatus,a5
> li      a4,18
> sd      a4,128(s1)
> csrc    sstatus,a5
> ...
> 
> 
> with "r", the generated code will be
> 
> ...
> csrs    sstatus,a5
> li      a4,18
> addi    s1,s1,128
> sd      a4,0(s1)
> csrc    sstatus,a5
> ...
> 
> As can be seen, "m" can make use of the 'offset' of
> sd, so save one instruction.
> 
> > 
> > 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