linux-kernel - Re: [PATCH 5/5] s390/uaccess: add cmpxchg_user

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, 09 Nov 2022 16:46:29 +0100
From:   Janis Schoetterl-Glausch <scgl@...ux.ibm.com>
To:     Heiko Carstens <hca@...ux.ibm.com>
Cc:     Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Janosch Frank <frankja@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        David Hildenbrand <david@...hat.com>,
        Jonathan Corbet <corbet@....net>, kvm@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-s390@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Shuah Khan <shuah@...nel.org>,
        Sven Schnelle <svens@...ux.ibm.com>
Subject: Re: [PATCH 5/5] s390/uaccess: add cmpxchg_user_key()

On Wed, 2022-11-02 at 15:19 +0100, Heiko Carstens wrote:
> Add cmpxchg_user_key() which allows to execute a compare and exchange
> on a user space address. This allows also to specify a storage key
> which makes sure that key-controlled protection is considered.
> 
> This is based on a patch written by Janis Schoetterl-Glausch.
> 
> Link: https://lore.kernel.org/all/20220930210751.225873-2-scgl@linux.ibm.com
> Cc: Janis Schoetterl-Glausch <scgl@...ux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@...ux.ibm.com>
> ---
>  arch/s390/include/asm/uaccess.h | 183 ++++++++++++++++++++++++++++++++
>  1 file changed, 183 insertions(+)
> 
> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
> index f7038b800cc3..9bbdecb80e06 100644
> --- a/arch/s390/include/asm/uaccess.h
> +++ b/arch/s390/include/asm/uaccess.h
> @@ -390,4 +390,187 @@ do {									\
>  		goto err_label;						\
>  } while (0)
>  
> +void __cmpxchg_user_key_called_with_bad_pointer(void);
> +
> +static __always_inline int __cmpxchg_user_key(unsigned long address, void *uval,
> +					      __uint128_t old, __uint128_t new,
> +					      unsigned long key, int size)
> +{
> +	int rc = 0;
> +
> +	switch (size) {
> +	case 1: {
> +		unsigned int prev, tmp, shift;
> +
> +		shift = (3 ^ (address & 3)) << 3;
> +		address ^= address & 3;
> +		asm volatile(
> +			"	spka	0(%[key])\n"
> +			"	sacf	256\n"
> +			"0:	l	%[prev],%[address]\n"
> +			"1:	nr	%[prev],%[mask]\n"
> +			"	lr	%[tmp],%[prev]\n"
> +			"	or	%[prev],%[old]\n"
> +			"	or	%[tmp],%[new]\n"
> +			"2:	cs	%[prev],%[tmp],%[address]\n"
> +			"3:	jnl	4f\n"
> +			"	xr	%[tmp],%[prev]\n"
> +			"	nr	%[tmp],%[mask]\n"

Are you only entertaining cosmetic changes to cmpxchg.h?
The loop condition being imprecise seems non-ideal.

> +			"	jnz	1b\n"
> +			"4:	sacf	768\n"
> +			"	spka	%[default_key]\n"
> +			EX_TABLE_UA_LOAD_REG(0b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(1b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(2b, 4b, %[rc], %[prev])
> +			EX_TABLE_UA_LOAD_REG(3b, 4b, %[rc], %[prev])
> +			: [rc] "+&d" (rc),
> +			  [prev] "=&d" (prev),
> +			  [tmp] "=&d" (tmp),
> +			  [address] "+Q" (*(int *)address)
> +			: [old] "d" (((unsigned int)old & 0xff) << shift),
> +			  [new] "d" (((unsigned int)new & 0xff) << shift),
> +			  [mask] "d" (~(0xff << shift)),
> +			  [key] "a" (key),

Why did you get rid of the << 4 shift?
That's inconsistent with the other uaccess functions that take an access key.

> +			  [default_key] "J" (PAGE_DEFAULT_KEY)
> +			: "memory", "cc");
> +		*(unsigned char *)uval = prev >> shift;
> +		return rc;
> +	}

[...]