| Message ID | 20220210173747.217574-1-deller@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | parisc: Fix some apparent put_user() failures | expand |
On 2/10/22 18:37, 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 a function call inside a put_user(), e.g.: > put_user(vt_do_kdgkbmode(console), (int __user *)arg); > clobbered the error register (r29) and thus the put_user() call itself > seem to have failed. Rearranging the C instructions didn't helped, > because then sometimes the compiler choosed another register which would > break the fault handler. > > This patch now takes another and much smarter approach. > > Instead of hardcoding a specific register as fault register, leave the > decision up to the compiler. In the load/store assembly we initialize > this register with zero by using "copy %%r0,reg". > > In case a fault happens, the fault handler will now read this copy > instruction, extract the used fault register and store the -EFAULT > failure code in it. > > Reported-by: John David Anglin <dave.anglin@bell.net> > Signed-off-by: Helge Deller <deller@gmx.de> > Fixes: 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8") > Signed-off-by: Helge Deller <deller@gmx.de> > --- > arch/parisc/include/asm/uaccess.h | 35 ++++++++++++++++--------------- > arch/parisc/mm/fault.c | 26 ++++++++++++++++++----- > 2 files changed, 39 insertions(+), 22 deletions(-) > > diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h > index ebf8a845b017..8254f0f09e2e 100644 > --- a/arch/parisc/include/asm/uaccess.h > +++ b/arch/parisc/include/asm/uaccess.h > @@ -53,18 +53,15 @@ struct exception_table_entry { > /* > * ASM_EXCEPTIONTABLE_ENTRY_EFAULT() creates a special exception table entry > * (with lowest bit set) for which the fault handler in fixup_exception() will > - * load -EFAULT into %r29 for a read or write fault, and zeroes the target > - * register in case of a read fault in get_user(). > + * load -EFAULT into the error register for a read or write fault, and zeroes > + * the target register in case of a read fault in get_user(). > */ > -#define ASM_EXCEPTIONTABLE_REG 29 > -#define ASM_EXCEPTIONTABLE_VAR(__variable) \ > - register long __variable __asm__ ("r29") = 0 > #define ASM_EXCEPTIONTABLE_ENTRY_EFAULT( fault_addr, except_addr )\ > ASM_EXCEPTIONTABLE_ENTRY( fault_addr, except_addr + 1) > > #define __get_user_internal(sr, val, ptr) \ > ({ \ > - ASM_EXCEPTIONTABLE_VAR(__gu_err); \ > + register long __gu_err; \ > \ > switch (sizeof(*(ptr))) { \ > case 1: __get_user_asm(sr, val, "ldb", ptr); break; \ > @@ -86,11 +83,12 @@ struct exception_table_entry { > { \ > register long __gu_val; \ > \ > - __asm__("1: " ldx " 0(" sr "%2),%0\n" \ > + __asm__("\tcopy %%r0,%1\n" \ > + "1: " ldx " 0(" sr "%2),%0\n" \ > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > - : "=r"(__gu_val), "=r"(__gu_err) \ > - : "r"(ptr), "1"(__gu_err)); \ > + : "=&r"(__gu_val), "=&r"(__gu_err) \ > + : "r"(ptr)); \ > \ > (val) = (__force __typeof__(*(ptr))) __gu_val; \ > } > @@ -117,14 +115,15 @@ struct exception_table_entry { > __typeof__(*(ptr)) t; \ > } __gu_tmp; \ > \ > - __asm__(" copy %%r0,%R0\n" \ > + __asm__("\tcopy %%r0,%R0\n" \ > + "\tcopy %%r0,%1\n" \ > "1: ldw 0(" sr "%2),%0\n" \ > "2: ldw 4(" sr "%2),%R0\n" \ I just notice that this approach fails on such loads/stores where multiple ldw/stw statements are used. In this case we need to check the last two instructions for the copy assembly statement. Will fix and send updated patch. Helge > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ > - : "=&r"(__gu_tmp.l), "=r"(__gu_err) \ > - : "r"(ptr), "1"(__gu_err)); \ > + : "=&r"(__gu_tmp.l), "=&r"(__gu_err) \ > + : "r"(ptr)); \ > \ > (val) = __gu_tmp.t; \ > } > @@ -134,8 +133,8 @@ struct exception_table_entry { > > #define __put_user_internal(sr, x, ptr) \ > ({ \ > - ASM_EXCEPTIONTABLE_VAR(__pu_err); \ > __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ > + register long __pu_err; \ > \ > switch (sizeof(*(ptr))) { \ > case 1: __put_user_asm(sr, "stb", __x, ptr); break; \ > @@ -177,24 +176,26 @@ struct exception_table_entry { > > #define __put_user_asm(sr, stx, x, ptr) \ > __asm__ __volatile__ ( \ > + "\tcopy %%r0,%0\n" \ > "1: " stx " %2,0(" sr "%1)\n" \ > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > - : "=r"(__pu_err) \ > - : "r"(ptr), "r"(x), "0"(__pu_err)) > + : "=&r"(__pu_err) \ > + : "r"(ptr), "r"(x)) > > > #if !defined(CONFIG_64BIT) > > #define __put_user_asm64(sr, __val, ptr) do { \ > __asm__ __volatile__ ( \ > + "\tcopy %%r0,%0\n" \ > "1: stw %2,0(" sr "%1)\n" \ > "2: stw %R2,4(" sr "%1)\n" \ > "9:\n" \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ > ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ > - : "=r"(__pu_err) \ > - : "r"(ptr), "r"(__val), "0"(__pu_err)); \ > + : "=&r"(__pu_err) \ > + : "r"(ptr), "r"(__val)); \ > } while (0) > > #endif /* !defined(CONFIG_64BIT) */ > diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c > index e9eabf8f14d7..d8f07d7430b0 100644 > --- a/arch/parisc/mm/fault.c > +++ b/arch/parisc/mm/fault.c > @@ -148,18 +148,33 @@ int fixup_exception(struct pt_regs *regs) > * Fix up get_user() and put_user(). > * ASM_EXCEPTIONTABLE_ENTRY_EFAULT() sets the least-significant > * bit in the relative address of the fixup routine to indicate > - * that gr[ASM_EXCEPTIONTABLE_REG] should be loaded with > - * -EFAULT to report a userspace access error. > + * that the error register should be loaded with -EFAULT to > + * report a userspace access error. The error register is zeroed > + * before doing the load or store, so the exception handler can > + * read the "copy %%r0,reg" instruction to extract the register. > */ > if (fix->fixup & 1) { > - regs->gr[ASM_EXCEPTIONTABLE_REG] = -EFAULT; > + u32 treg; > > /* zero target register for get_user() */ > if (parisc_acctyp(0, regs->iir) == VM_READ) { > - int treg = regs->iir & 0x1f; > - BUG_ON(treg == 0); > + treg = regs->iir & 0x1f; > + if (WARN_ON(treg == 0)) > + goto wrong_get_put_user; > regs->gr[treg] = 0; > } > + > + /* get previous assembly statement */ > + __get_kernel_nofault(&treg, (regs->iaoq[0]-4) & ~3, > + u32, wrong_get_put_user); > + /* check assembly statement for: copy %r0,%rX */ > + if (WARN_ON((treg & 0xffffff00) != 0x08000200)) > + goto wrong_get_put_user; > + /* extract error register used for get_user/put_user */ > + treg &= 0x1f; > + if (WARN_ON(treg == 0)) > + goto wrong_get_put_user; > + regs->gr[treg] = -EFAULT; > } > > regs->iaoq[0] = (unsigned long)&fix->fixup + fix->fixup; > @@ -177,6 +192,7 @@ int fixup_exception(struct pt_regs *regs) > return 1; > } > > +wrong_get_put_user: > return 0; > } > > -- > 2.34.1 >
diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h index ebf8a845b017..8254f0f09e2e 100644 --- a/arch/parisc/include/asm/uaccess.h +++ b/arch/parisc/include/asm/uaccess.h @@ -53,18 +53,15 @@ struct exception_table_entry { /* * ASM_EXCEPTIONTABLE_ENTRY_EFAULT() creates a special exception table entry * (with lowest bit set) for which the fault handler in fixup_exception() will - * load -EFAULT into %r29 for a read or write fault, and zeroes the target - * register in case of a read fault in get_user(). + * load -EFAULT into the error register for a read or write fault, and zeroes + * the target register in case of a read fault in get_user(). */ -#define ASM_EXCEPTIONTABLE_REG 29 -#define ASM_EXCEPTIONTABLE_VAR(__variable) \ - register long __variable __asm__ ("r29") = 0 #define ASM_EXCEPTIONTABLE_ENTRY_EFAULT( fault_addr, except_addr )\ ASM_EXCEPTIONTABLE_ENTRY( fault_addr, except_addr + 1) #define __get_user_internal(sr, val, ptr) \ ({ \ - ASM_EXCEPTIONTABLE_VAR(__gu_err); \ + register long __gu_err; \ \ switch (sizeof(*(ptr))) { \ case 1: __get_user_asm(sr, val, "ldb", ptr); break; \ @@ -86,11 +83,12 @@ struct exception_table_entry { { \ register long __gu_val; \ \ - __asm__("1: " ldx " 0(" sr "%2),%0\n" \ + __asm__("\tcopy %%r0,%1\n" \ + "1: " ldx " 0(" sr "%2),%0\n" \ "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ - : "=r"(__gu_val), "=r"(__gu_err) \ - : "r"(ptr), "1"(__gu_err)); \ + : "=&r"(__gu_val), "=&r"(__gu_err) \ + : "r"(ptr)); \ \ (val) = (__force __typeof__(*(ptr))) __gu_val; \ } @@ -117,14 +115,15 @@ struct exception_table_entry { __typeof__(*(ptr)) t; \ } __gu_tmp; \ \ - __asm__(" copy %%r0,%R0\n" \ + __asm__("\tcopy %%r0,%R0\n" \ + "\tcopy %%r0,%1\n" \ "1: ldw 0(" sr "%2),%0\n" \ "2: ldw 4(" sr "%2),%R0\n" \ "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ - : "=&r"(__gu_tmp.l), "=r"(__gu_err) \ - : "r"(ptr), "1"(__gu_err)); \ + : "=&r"(__gu_tmp.l), "=&r"(__gu_err) \ + : "r"(ptr)); \ \ (val) = __gu_tmp.t; \ } @@ -134,8 +133,8 @@ struct exception_table_entry { #define __put_user_internal(sr, x, ptr) \ ({ \ - ASM_EXCEPTIONTABLE_VAR(__pu_err); \ __typeof__(*(ptr)) __x = (__typeof__(*(ptr)))(x); \ + register long __pu_err; \ \ switch (sizeof(*(ptr))) { \ case 1: __put_user_asm(sr, "stb", __x, ptr); break; \ @@ -177,24 +176,26 @@ struct exception_table_entry { #define __put_user_asm(sr, stx, x, ptr) \ __asm__ __volatile__ ( \ + "\tcopy %%r0,%0\n" \ "1: " stx " %2,0(" sr "%1)\n" \ "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ - : "=r"(__pu_err) \ - : "r"(ptr), "r"(x), "0"(__pu_err)) + : "=&r"(__pu_err) \ + : "r"(ptr), "r"(x)) #if !defined(CONFIG_64BIT) #define __put_user_asm64(sr, __val, ptr) do { \ __asm__ __volatile__ ( \ + "\tcopy %%r0,%0\n" \ "1: stw %2,0(" sr "%1)\n" \ "2: stw %R2,4(" sr "%1)\n" \ "9:\n" \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 9b) \ ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 9b) \ - : "=r"(__pu_err) \ - : "r"(ptr), "r"(__val), "0"(__pu_err)); \ + : "=&r"(__pu_err) \ + : "r"(ptr), "r"(__val)); \ } while (0) #endif /* !defined(CONFIG_64BIT) */ diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c index e9eabf8f14d7..d8f07d7430b0 100644 --- a/arch/parisc/mm/fault.c +++ b/arch/parisc/mm/fault.c @@ -148,18 +148,33 @@ int fixup_exception(struct pt_regs *regs) * Fix up get_user() and put_user(). * ASM_EXCEPTIONTABLE_ENTRY_EFAULT() sets the least-significant * bit in the relative address of the fixup routine to indicate - * that gr[ASM_EXCEPTIONTABLE_REG] should be loaded with - * -EFAULT to report a userspace access error. + * that the error register should be loaded with -EFAULT to + * report a userspace access error. The error register is zeroed + * before doing the load or store, so the exception handler can + * read the "copy %%r0,reg" instruction to extract the register. */ if (fix->fixup & 1) { - regs->gr[ASM_EXCEPTIONTABLE_REG] = -EFAULT; + u32 treg; /* zero target register for get_user() */ if (parisc_acctyp(0, regs->iir) == VM_READ) { - int treg = regs->iir & 0x1f; - BUG_ON(treg == 0); + treg = regs->iir & 0x1f; + if (WARN_ON(treg == 0)) + goto wrong_get_put_user; regs->gr[treg] = 0; } + + /* get previous assembly statement */ + __get_kernel_nofault(&treg, (regs->iaoq[0]-4) & ~3, + u32, wrong_get_put_user); + /* check assembly statement for: copy %r0,%rX */ + if (WARN_ON((treg & 0xffffff00) != 0x08000200)) + goto wrong_get_put_user; + /* extract error register used for get_user/put_user */ + treg &= 0x1f; + if (WARN_ON(treg == 0)) + goto wrong_get_put_user; + regs->gr[treg] = -EFAULT; } regs->iaoq[0] = (unsigned long)&fix->fixup + fix->fixup; @@ -177,6 +192,7 @@ int fixup_exception(struct pt_regs *regs) return 1; } +wrong_get_put_user: return 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 a function call inside a put_user(), e.g.: put_user(vt_do_kdgkbmode(console), (int __user *)arg); clobbered the error register (r29) and thus the put_user() call itself seem to have failed. Rearranging the C instructions didn't helped, because then sometimes the compiler choosed another register which would break the fault handler. This patch now takes another and much smarter approach. Instead of hardcoding a specific register as fault register, leave the decision up to the compiler. In the load/store assembly we initialize this register with zero by using "copy %%r0,reg". In case a fault happens, the fault handler will now read this copy instruction, extract the used fault register and store the -EFAULT failure code in it. Reported-by: John David Anglin <dave.anglin@bell.net> Signed-off-by: Helge Deller <deller@gmx.de> Fixes: 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8") Signed-off-by: Helge Deller <deller@gmx.de> --- arch/parisc/include/asm/uaccess.h | 35 ++++++++++++++++--------------- arch/parisc/mm/fault.c | 26 ++++++++++++++++++----- 2 files changed, 39 insertions(+), 22 deletions(-) -- 2.34.1