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: <f1218075-94a5-429a-ab35-f47d3e79e80e@tenstorrent.com>
Date: Fri, 31 Jan 2025 12:31:38 +1100
From: Cyril Bur <cyrilbur@...storrent.com>
To: Jessica Clarke <jrtc27@...c27.com>, charlie@...osinc.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/1/2025 9:34 am, Jessica Clarke wrote:
> 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

Good points, I agree. I am happy to respin this patch move the =m to 
output constraint.

@Charlie Please let me know if you would like me to keep your tags on 
the tweaked version.

Thanks,

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