| Message ID | 1399270438-26181-1-git-send-email-a.ryabinin@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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);
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
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 >
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))) { \
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(-)