| Message ID | YgQGzle/mBRK9lBc@p100 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | parisc: Fix user access miscompilation | expand |
On 2022-02-09 1:24 p.m., Helge Deller wrote: > After commit 4b9d2a731c3d ("parisc: Switch user access functions > to signal errors in r29 instead of r8") bash suddenly started > to report those warnings after login: > > -bash: cannot set terminal process group (-1): Bad file descriptor > -bash: no job control in this shell > > It turned out, that another function call inside a put_user(), e.g.: > put_user(vt_do_kdgkbmode(console), (int __user *)arg); > clobbered the error register (r29) and thus failed. > Avoid this miscompilation by first calculate the value in > __put_user() before calling __put_user_internal() to actually > write the value to user space. Couldn't the same issue occur with ptr argument in these macros? > > Additionally, prefer the "+" constraint on pu_err and gu_err registers to tell > the compiler that those operands are both read and written by the assembly > instruction. > > Signed-off-by: Helge Deller <deller@gmx.de> > Fixes: 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8") > > diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h > index ebf8a845b017..68f5c1eaaa6f 100644 > --- a/arch/parisc/include/asm/uaccess.h > +++ b/arch/parisc/include/asm/uaccess.h > @@ -89,7 +89,7 @@ struct exception_table_entry { > __asm__("1: " ldx " 0(" sr "%2),%0\n" \ > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > - : "=r"(__gu_val), "=r"(__gu_err) \ > + : "=&r"(__gu_val), "+r"(__gu_err) \ I don't believe the early clobber is needed on __gnu_val. > : "r"(ptr), "1"(__gu_err)); \ > \ > (val) = (__force __typeof__(*(ptr))) __gu_val; \ > @@ -123,7 +123,7 @@ struct exception_table_entry { > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ > - : "=&r"(__gu_tmp.l), "=r"(__gu_err) \ > + : "=&r"(__gu_tmp.l), "+r"(__gu_err) \ > : "r"(ptr), "1"(__gu_err)); \ > \ > (val) = __gu_tmp.t; \ > @@ -132,10 +132,9 @@ struct exception_table_entry { > #endif /* !defined(CONFIG_64BIT) */ > > > -#define __put_user_internal(sr, x, ptr) \ > +#define __put_user_internal(sr, __x, ptr) \ > ({ \ > ASM_EXCEPTIONTABLE_VAR(__pu_err); \ > - __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ > \ > switch (sizeof(*(ptr))) { \ > case 1: __put_user_asm(sr, "stb", __x, ptr); break; \ > @@ -150,7 +149,9 @@ struct exception_table_entry { > > #define __put_user(x, ptr) \ > ({ \ > - __put_user_internal("%%sr3,", x, ptr); \ > + __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ Whitespace? > + \ > + __put_user_internal("%%sr3,", __x, ptr); \ > }) > > #define __put_kernel_nofault(dst, src, type, err_label) \ > @@ -180,7 +181,7 @@ struct exception_table_entry { > "1: " stx " %2,0(" sr "%1)\n" \ > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > - : "=r"(__pu_err) \ > + : "+r"(__pu_err) \ > : "r"(ptr), "r"(x), "0"(__pu_err)) > > > @@ -193,7 +194,7 @@ struct exception_table_entry { > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ > - : "=r"(__pu_err) \ > + : "+r"(__pu_err) \ > : "r"(ptr), "r"(__val), "0"(__pu_err)); \ > } while (0) > Dave
On 2/10/22 11:26, John David Anglin wrote: > On 2022-02-09 1:24 p.m., Helge Deller wrote: >> After commit 4b9d2a731c3d ("parisc: Switch user access functions >> to signal errors in r29 instead of r8") bash suddenly started >> to report those warnings after login: >> >> -bash: cannot set terminal process group (-1): Bad file descriptor >> -bash: no job control in this shell >> >> It turned out, that another function call inside a put_user(), e.g.: >> put_user(vt_do_kdgkbmode(console), (int __user *)arg); >> clobbered the error register (r29) and thus failed. >> Avoid this miscompilation by first calculate the value in >> __put_user() before calling __put_user_internal() to actually >> write the value to user space. > Couldn't the same issue occur with ptr argument in these macros? Theoretically, yes. Haven't seen it though. >> Additionally, prefer the "+" constraint on pu_err and gu_err registers to tell >> the compiler that those operands are both read and written by the assembly >> instruction. >> >> Signed-off-by: Helge Deller <deller@gmx.de> >> Fixes: 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8") >> >> diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h >> index ebf8a845b017..68f5c1eaaa6f 100644 >> --- a/arch/parisc/include/asm/uaccess.h >> +++ b/arch/parisc/include/asm/uaccess.h >> @@ -89,7 +89,7 @@ struct exception_table_entry { >> __asm__("1: " ldx " 0(" sr "%2),%0\n" \ >> "9:\n" \ >> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ >> - : "=r"(__gu_val), "=r"(__gu_err) \ >> + : "=&r"(__gu_val), "+r"(__gu_err) \ > I don't believe the early clobber is needed on __gnu_val. Right. >> : "r"(ptr), "1"(__gu_err)); \ >> \ >> (val) = (__force __typeof__(*(ptr))) __gu_val; \ >> @@ -123,7 +123,7 @@ struct exception_table_entry { >> "9:\n" \ >> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ >> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ >> - : "=&r"(__gu_tmp.l), "=r"(__gu_err) \ >> + : "=&r"(__gu_tmp.l), "+r"(__gu_err) \ >> : "r"(ptr), "1"(__gu_err)); \ >> \ >> (val) = __gu_tmp.t; \ >> @@ -132,10 +132,9 @@ struct exception_table_entry { >> #endif /* !defined(CONFIG_64BIT) */ >> >> >> -#define __put_user_internal(sr, x, ptr) \ >> +#define __put_user_internal(sr, __x, ptr) \ >> ({ \ >> ASM_EXCEPTIONTABLE_VAR(__pu_err); \ >> - __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ >> \ >> switch (sizeof(*(ptr))) { \ >> case 1: __put_user_asm(sr, "stb", __x, ptr); break; \ >> @@ -150,7 +149,9 @@ struct exception_table_entry { >> >> #define __put_user(x, ptr) \ >> ({ \ >> - __put_user_internal("%%sr3,", x, ptr); \ >> + __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ > Whitespace? Will fix. >> + \ >> + __put_user_internal("%%sr3,", __x, ptr); \ >> }) >> >> #define __put_kernel_nofault(dst, src, type, err_label) \ >> @@ -180,7 +181,7 @@ struct exception_table_entry { >> "1: " stx " %2,0(" sr "%1)\n" \ >> "9:\n" \ >> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ >> - : "=r"(__pu_err) \ >> + : "+r"(__pu_err) \ >> : "r"(ptr), "r"(x), "0"(__pu_err)) >> >> >> @@ -193,7 +194,7 @@ struct exception_table_entry { >> "9:\n" \ >> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ >> ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ >> - : "=r"(__pu_err) \ >> + : "+r"(__pu_err) \ >> : "r"(ptr), "r"(__val), "0"(__pu_err)); \ >> } while (0) Overall, this current patch turned out to not cover all cases. I think it's too complicated to really try prevent the compiler to optimize it here, so I'll send out a new patch shortly which simply initializes __gu_err and __pu_err in the assembly statement itself. My other codings to avoid that would increase the code a lot, and I'm not sure if it's even then possible to avoid the failure always. Helge
diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h index ebf8a845b017..68f5c1eaaa6f 100644 --- a/arch/parisc/include/asm/uaccess.h +++ b/arch/parisc/include/asm/uaccess.h @@ -89,7 +89,7 @@ struct exception_table_entry { __asm__("1: " ldx " 0(" sr "%2),%0\n" \ "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ - : "=r"(__gu_val), "=r"(__gu_err) \ + : "=&r"(__gu_val), "+r"(__gu_err) \ : "r"(ptr), "1"(__gu_err)); \ \ (val) = (__force __typeof__(*(ptr))) __gu_val; \ @@ -123,7 +123,7 @@ struct exception_table_entry { "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ - : "=&r"(__gu_tmp.l), "=r"(__gu_err) \ + : "=&r"(__gu_tmp.l), "+r"(__gu_err) \ : "r"(ptr), "1"(__gu_err)); \ \ (val) = __gu_tmp.t; \ @@ -132,10 +132,9 @@ struct exception_table_entry { #endif /* !defined(CONFIG_64BIT) */ -#define __put_user_internal(sr, x, ptr) \ +#define __put_user_internal(sr, __x, ptr) \ ({ \ ASM_EXCEPTIONTABLE_VAR(__pu_err); \ - __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ \ switch (sizeof(*(ptr))) { \ case 1: __put_user_asm(sr, "stb", __x, ptr); break; \ @@ -150,7 +149,9 @@ struct exception_table_entry { #define __put_user(x, ptr) \ ({ \ - __put_user_internal("%%sr3,", x, ptr); \ + __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ + \ + __put_user_internal("%%sr3,", __x, ptr); \ }) #define __put_kernel_nofault(dst, src, type, err_label) \ @@ -180,7 +181,7 @@ struct exception_table_entry { "1: " stx " %2,0(" sr "%1)\n" \ "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ - : "=r"(__pu_err) \ + : "+r"(__pu_err) \ : "r"(ptr), "r"(x), "0"(__pu_err)) @@ -193,7 +194,7 @@ struct exception_table_entry { "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ - : "=r"(__pu_err) \ + : "+r"(__pu_err) \ : "r"(ptr), "r"(__val), "0"(__pu_err)); \ } while (0)
After commit 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8") bash suddenly started to report those warnings after login: -bash: cannot set terminal process group (-1): Bad file descriptor -bash: no job control in this shell It turned out, that another function call inside a put_user(), e.g.: put_user(vt_do_kdgkbmode(console), (int __user *)arg); clobbered the error register (r29) and thus failed. Avoid this miscompilation by first calculate the value in __put_user() before calling __put_user_internal() to actually write the value to user space. Additionally, prefer the "+" constraint on pu_err and gu_err registers to tell the compiler that those operands are both read and written by the assembly instruction. Signed-off-by: Helge Deller <deller@gmx.de> Fixes: 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8")