[PATCH] arm: put_user: fix possible data corruption in put_user

[PATCH] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

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.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.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);			\
 		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

[PATCH] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

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.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.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);			\
 		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

[PATCH] arm: put_user: fix possible data corruption in put_user

[Russell King - ARM Linux]

On Mon, May 05, 2014 at 10:13:58AM +0400, 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.

This and the patch make no sense.  You talk about r2, but you're doing
nothing with r2 in the patch.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH] arm: put_user: fix possible data corruption in put_user

[Russell King - ARM Linux]

On Mon, May 05, 2014 at 10:13:58AM +0400, 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.

This and the patch make no sense.  You talk about r2, but you're doing
nothing with r2 in the patch.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

On 05/05/14 13:01, Russell King - ARM Linux wrote:
> On Mon, May 05, 2014 at 10:13:58AM +0400, 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.
> 
> This and the patch make no sense.  You talk about r2, but you're doing
> nothing with r2 in the patch.
> 

No, you didn't get it. I'll try to explain better.

Lets consider following example:

unsigned int __user *get_address(void);
...
	put_user(1, get_address());
...

Pay attention that in get_address function register r2  may be used.
In above example, without my patch, put_user macro will be expanded to the following code:

...
	register const unsigned int __r2 asm("r2") = (1);
	register const unsigned int __user *__p asm("r0") = (get_address());
...

At first we put value 1 into r2 register. After that get_address is called, and clobbers r2 register.
This means that after assignment to variable __p, register r2 may no longer contain a valid value - 1.

My patch put get_address calls befor the assignment of (x) to __r2.
With my patch, put_user macro will be expanded to the following code:

...
	const unsigned int __user *tmp_p = (get_address());
	register const unsigned int __r2 asm("r2") = (1);
	register const unsigned int __user *__p asm("r0") = tmp_p;
...

In this time get_address() call happens before loading 1 to r2, so it won't be corrupted.


Here is the full code of test, so anyone could check.

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/uaccess.h>

unsigned int x = 0;
unsigned int y = 0;

/* get_address returns address of x, and clobbers r2 register */
unsigned int __user *get_address(void)
{
	mm_segment_t oldfs;
	oldfs = get_fs();
	set_fs(get_ds());
	put_user(2, &y); /* this put_user call will put value 2 in register r2 */
	set_fs(oldfs);
	return &x;
}

static __init int test_init(void)
{
	mm_segment_t oldfs;
	oldfs = get_fs();
	set_fs(get_ds());
	put_user(1, get_address()); /* put 1 to x */
	set_fs(oldfs);

	printk("\nput_user_test: value %x\n\n", *get_address()); /* this will print "put_user_test: value 2" instead of "put_user_test: value 1"
	return 0;
}

module_init(test_init);

Re: [PATCH] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

On 05/05/14 13:01, Russell King - ARM Linux wrote:
> On Mon, May 05, 2014 at 10:13:58AM +0400, 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.
> 
> This and the patch make no sense.  You talk about r2, but you're doing
> nothing with r2 in the patch.
> 

No, you didn't get it. I'll try to explain better.

Lets consider following example:

unsigned int __user *get_address(void);
...
	put_user(1, get_address());
...

Pay attention that in get_address function register r2  may be used.
In above example, without my patch, put_user macro will be expanded to the following code:

...
	register const unsigned int __r2 asm("r2") = (1);
	register const unsigned int __user *__p asm("r0") = (get_address());
...

At first we put value 1 into r2 register. After that get_address is called, and clobbers r2 register.
This means that after assignment to variable __p, register r2 may no longer contain a valid value - 1.

My patch put get_address calls befor the assignment of (x) to __r2.
With my patch, put_user macro will be expanded to the following code:

...
	const unsigned int __user *tmp_p = (get_address());
	register const unsigned int __r2 asm("r2") = (1);
	register const unsigned int __user *__p asm("r0") = tmp_p;
...

In this time get_address() call happens before loading 1 to r2, so it won't be corrupted.


Here is the full code of test, so anyone could check.

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/uaccess.h>

unsigned int x = 0;
unsigned int y = 0;

/* get_address returns address of x, and clobbers r2 register */
unsigned int __user *get_address(void)
{
	mm_segment_t oldfs;
	oldfs = get_fs();
	set_fs(get_ds());
	put_user(2, &y); /* this put_user call will put value 2 in register r2 */
	set_fs(oldfs);
	return &x;
}

static __init int test_init(void)
{
	mm_segment_t oldfs;
	oldfs = get_fs();
	set_fs(get_ds());
	put_user(1, get_address()); /* put 1 to x */
	set_fs(oldfs);

	printk("\nput_user_test: value %x\n\n", *get_address()); /* this will print "put_user_test: value 2" instead of "put_user_test: value 1"
	return 0;
}

module_init(test_init);

[PATCH] arm: put_user: fix possible data corruption in put_user

[Nicolas Pitre]

On Mon, 5 May 2014, Andrey Ryabinin wrote:

> On 05/05/14 13:01, Russell King - ARM Linux wrote:
> > On Mon, May 05, 2014 at 10:13:58AM +0400, 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.
> > 
> > This and the patch make no sense.  You talk about r2, but you're doing
> > nothing with r2 in the patch.
> > 
> 
> No, you didn't get it. I'll try to explain better.
> 
> Lets consider following example:
[...]

Thanks for the test code.  I do confirm there is indeed a problem.

I'm trying to make sure your patch is actually the best fix.


Nicolas

Re: [PATCH] arm: put_user: fix possible data corruption in put_user

[Nicolas Pitre]

On Mon, 5 May 2014, Andrey Ryabinin wrote:

> On 05/05/14 13:01, Russell King - ARM Linux wrote:
> > On Mon, May 05, 2014 at 10:13:58AM +0400, 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.
> > 
> > This and the patch make no sense.  You talk about r2, but you're doing
> > nothing with r2 in the patch.
> > 
> 
> No, you didn't get it. I'll try to explain better.
> 
> Lets consider following example:
[...]

Thanks for the test code.  I do confirm there is indeed a problem.

I'm trying to make sure your patch is actually the best fix.


Nicolas

[PATCH] arm: put_user: fix possible data corruption in put_user

[Nicolas Pitre]

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@samsung.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@linaro.org>

... and add "Cc: stable at vger.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
> 

Re: [PATCH] arm: put_user: fix possible data corruption in put_user

[Nicolas Pitre]

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@samsung.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@linaro.org>

... and add "Cc: stable@vger.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
> 

[PATCHv2] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

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.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
Reviewed-by: Nicolas Pitre <nico@linaro.org>
Cc: stable at vger.kernel.org
---
Since v1:
 - tmp_p variable renamed to __tmp_p
 - added Reviewed-by tag
 - added Cc: stable at vger.kernel.org

 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..75d9579 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);		\
 		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.5.5

[PATCHv2] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

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.

Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
Reviewed-by: Nicolas Pitre <nico@linaro.org>
Cc: stable@vger.kernel.org
---
Since v1:
 - tmp_p variable renamed to __tmp_p
 - added Reviewed-by tag
 - added Cc: stable@vger.kernel.org

 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..75d9579 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);		\
 		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.5.5

[PATCHv2] arm: put_user: fix possible data corruption in put_user

[Nicolas Pitre]

On Tue, 6 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.
> 
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> Reviewed-by: Nicolas Pitre <nico@linaro.org>
> Cc: stable at vger.kernel.org

Please push this patch into Russell's patch system.

Thanks.

> ---
> Since v1:
>  - tmp_p variable renamed to __tmp_p
>  - added Reviewed-by tag
>  - added Cc: stable at vger.kernel.org
> 
>  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..75d9579 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);		\
>  		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.5.5
> 

Re: [PATCHv2] arm: put_user: fix possible data corruption in put_user

[Nicolas Pitre]

On Tue, 6 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.
> 
> Signed-off-by: Andrey Ryabinin <a.ryabinin@samsung.com>
> Reviewed-by: Nicolas Pitre <nico@linaro.org>
> Cc: stable@vger.kernel.org

Please push this patch into Russell's patch system.

Thanks.

> ---
> Since v1:
>  - tmp_p variable renamed to __tmp_p
>  - added Reviewed-by tag
>  - added Cc: stable@vger.kernel.org
> 
>  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..75d9579 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);		\
>  		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.5.5
> 

[PATCHv2] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

On 05/07/14 00:13, Nicolas Pitre wrote:
> Please push this patch into Russell's patch system.
> 
> Thanks.
> 

Done - http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8051/1

Thanks.

Re: [PATCHv2] arm: put_user: fix possible data corruption in put_user

[Andrey Ryabinin]

On 05/07/14 00:13, Nicolas Pitre wrote:
> Please push this patch into Russell's patch system.
> 
> Thanks.
> 

Done - http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8051/1

Thanks.