* [PATCH] parisc: Fix user access miscompilation
@ 2022-02-09 18:24 Helge Deller
2022-02-10 10:26 ` John David Anglin
0 siblings, 1 reply; 3+ messages in thread
From: Helge Deller @ 2022-02-09 18:24 UTC (permalink / raw)
To: linux-parisc, James Bottomley, John David Anglin, Sven Schnelle
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")
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)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] parisc: Fix user access miscompilation
2022-02-09 18:24 [PATCH] parisc: Fix user access miscompilation Helge Deller
@ 2022-02-10 10:26 ` John David Anglin
2022-02-10 10:56 ` Helge Deller
0 siblings, 1 reply; 3+ messages in thread
From: John David Anglin @ 2022-02-10 10:26 UTC (permalink / raw)
To: Helge Deller, linux-parisc, James Bottomley, Sven Schnelle
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
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] parisc: Fix user access miscompilation
2022-02-10 10:26 ` John David Anglin
@ 2022-02-10 10:56 ` Helge Deller
0 siblings, 0 replies; 3+ messages in thread
From: Helge Deller @ 2022-02-10 10:56 UTC (permalink / raw)
To: John David Anglin, linux-parisc, James Bottomley, Sven Schnelle
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-10 10:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09 18:24 [PATCH] parisc: Fix user access miscompilation Helge Deller
2022-02-10 10:26 ` John David Anglin
2022-02-10 10:56 ` Helge Deller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.