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@xxxxxxxx>
> Signed-off-by: Helge Deller <deller@xxxxxx>
> Fixes: 4b9d2a731c3d ("parisc: Switch user access functions to signal errors in r29 instead of r8")
> Signed-off-by: Helge Deller <deller@xxxxxx>
> ---
> 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
>
