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:	Mon, 5 May 2014 10:15:30 -0400 (EDT)
From:	Nicolas Pitre <nicolas.pitre@...aro.org>
To:	Andrey Ryabinin <a.ryabinin@...sung.com>
cc:	linux@....linux.org.uk, will.deacon@....com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm: put_user: fix possible data corruption in
 put_user

On Mon, 5 May 2014, Andrey Ryabinin wrote:

> According to arm procedure call standart r2 register is call-cloberred.
> So after the result of x expression was put into r2 any following
> function call in p may overwrite r2. To fix this, the result of p
> expression must be saved to the temporary variable before the
> assigment x expression to __r2.

As subtle as it is, this appears to be exact.

However ...

> Signed-off-by: Andrey Ryabinin <a.ryabinin@...sung.com>
> ---
>  arch/arm/include/asm/uaccess.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index 12c3a5d..4b584ac 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -171,8 +171,9 @@ extern int __put_user_8(void *, unsigned long long);
>  #define __put_user_check(x,p)							\
>  	({								\
>  		unsigned long __limit = current_thread_info()->addr_limit - 1; \
> +		const typeof(*(p)) __user *tmp_p = (p);			\

Please use __tmp_p here rather than tmp_p as this could conflict with a 
variable of the same name in the calling context.  After that change you 
may add:

Reviewed-by: Nicolas Pitre <nico@...aro.org>

... and add "Cc: stable@...r.kernel.org" as well.

I confirm that, with this patch, the generated assembly from the test 
case is identical except for the added initialization of r2 which is 
optimized away otherwise.

Looking at all the other occurrences of register specified variables, 
they appear safe. We already encountered this issue as illustrated by 
commit 98d4ded60b but apparently failed to see the possibility for the 
same problem to occur elsewhere at the time.

>  		register const typeof(*(p)) __r2 asm("r2") = (x);	\
> -		register const typeof(*(p)) __user *__p asm("r0") = (p);\
> +		register const typeof(*(p)) __user *__p asm("r0") = tmp_p; \
>  		register unsigned long __l asm("r1") = __limit;		\
>  		register int __e asm("r0");				\
>  		switch (sizeof(*(__p))) {				\
> -- 
> 1.8.3.2
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists