Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

uaccess: Provide ASM GOTO safe wrappers for unsafe_*_user()

ASM GOTO is miscompiled by GCC when it is used inside a auto cleanup scope:

bool foo(u32 __user *p, u32 val)
{
scoped_guard(pagefault)
unsafe_put_user(val, p, efault);
return true;
efault:
return false;
}

e80: e8 00 00 00 00 call e85 <foo+0x5>
e85: 65 48 8b 05 00 00 00 00 mov %gs:0x0(%rip),%rax
e8d: 83 80 04 14 00 00 01 addl $0x1,0x1404(%rax) // pf_disable++
e94: 89 37 mov %esi,(%rdi)
e96: 83 a8 04 14 00 00 01 subl $0x1,0x1404(%rax) // pf_disable--
e9d: b8 01 00 00 00 mov $0x1,%eax // success
ea2: e9 00 00 00 00 jmp ea7 <foo+0x27> // ret
ea7: 31 c0 xor %eax,%eax // fail
ea9: e9 00 00 00 00 jmp eae <foo+0x2e> // ret

which is broken as it leaks the pagefault disable counter on failure.

Clang at least fails the build.

Linus suggested to add a local label into the macro scope and let that
jump to the actual caller supplied error label.

__label__ local_label; \
arch_unsafe_get_user(x, ptr, local_label); \
if (0) { \
local_label: \
goto label; \

That works for both GCC and clang.

clang:

c80: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
c85: 65 48 8b 0c 25 00 00 00 00 mov %gs:0x0,%rcx
c8e: ff 81 04 14 00 00 incl 0x1404(%rcx) // pf_disable++
c94: 31 c0 xor %eax,%eax // set retval to false
c96: 89 37 mov %esi,(%rdi) // write
c98: b0 01 mov $0x1,%al // set retval to true
c9a: ff 89 04 14 00 00 decl 0x1404(%rcx) // pf_disable--
ca0: 2e e9 00 00 00 00 cs jmp ca6 <foo+0x26> // ret

The exception table entry points correctly to c9a

GCC:

f70: e8 00 00 00 00 call f75 <baz+0x5>
f75: 65 48 8b 05 00 00 00 00 mov %gs:0x0(%rip),%rax
f7d: 83 80 04 14 00 00 01 addl $0x1,0x1404(%rax) // pf_disable++
f84: 8b 17 mov (%rdi),%edx
f86: 89 16 mov %edx,(%rsi)
f88: 83 a8 04 14 00 00 01 subl $0x1,0x1404(%rax) // pf_disable--
f8f: b8 01 00 00 00 mov $0x1,%eax // success
f94: e9 00 00 00 00 jmp f99 <baz+0x29> // ret
f99: 83 a8 04 14 00 00 01 subl $0x1,0x1404(%rax) // pf_disable--
fa0: 31 c0 xor %eax,%eax // fail
fa2: e9 00 00 00 00 jmp fa7 <baz+0x37> // ret

The exception table entry points correctly to f99

So both compilers optimize out the extra goto and emit correct and
efficient code.

Provide a generic wrapper to do that to avoid modifying all the affected
architecture specific implementation with that workaround.

The only change required for architectures is to rename unsafe_*_user() to
arch_unsafe_*_user(). That's done in subsequent changes.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Link: https://patch.msgid.link/877bweujtn.ffs@tglx

authored by

Thomas Gleixner and committed by
Peter Zijlstra
3eb6660f 44c5b676

+68 -4
+68 -4
include/linux/uaccess.h
··· 518 518 long count); 519 519 long strnlen_user_nofault(const void __user *unsafe_addr, long count); 520 520 521 - #ifndef __get_kernel_nofault 521 + #ifdef arch_get_kernel_nofault 522 + /* 523 + * Wrap the architecture implementation so that @label can be outside of a 524 + * cleanup() scope. A regular C goto works correctly, but ASM goto does 525 + * not. Clang rejects such an attempt, but GCC silently emits buggy code. 526 + */ 527 + #define __get_kernel_nofault(dst, src, type, label) \ 528 + do { \ 529 + __label__ local_label; \ 530 + arch_get_kernel_nofault(dst, src, type, local_label); \ 531 + if (0) { \ 532 + local_label: \ 533 + goto label; \ 534 + } \ 535 + } while (0) 536 + 537 + #define __put_kernel_nofault(dst, src, type, label) \ 538 + do { \ 539 + __label__ local_label; \ 540 + arch_put_kernel_nofault(dst, src, type, local_label); \ 541 + if (0) { \ 542 + local_label: \ 543 + goto label; \ 544 + } \ 545 + } while (0) 546 + 547 + #elif !defined(__get_kernel_nofault) /* arch_get_kernel_nofault */ 548 + 522 549 #define __get_kernel_nofault(dst, src, type, label) \ 523 550 do { \ 524 551 type __user *p = (type __force __user *)(src); \ ··· 562 535 if (__put_user(data, p)) \ 563 536 goto label; \ 564 537 } while (0) 565 - #endif 538 + 539 + #endif /* !__get_kernel_nofault */ 566 540 567 541 /** 568 542 * get_kernel_nofault(): safely attempt to read from a location ··· 577 549 copy_from_kernel_nofault(&(val), __gk_ptr, sizeof(val));\ 578 550 }) 579 551 580 - #ifndef user_access_begin 552 + #ifdef user_access_begin 553 + 554 + #ifdef arch_unsafe_get_user 555 + /* 556 + * Wrap the architecture implementation so that @label can be outside of a 557 + * cleanup() scope. A regular C goto works correctly, but ASM goto does 558 + * not. Clang rejects such an attempt, but GCC silently emits buggy code. 559 + * 560 + * Some architectures use internal local labels already, but this extra 561 + * indirection here is harmless because the compiler optimizes it out 562 + * completely in any case. This construct just ensures that the ASM GOTO 563 + * target is always in the local scope. The C goto 'label' works correctly 564 + * when leaving a cleanup() scope. 565 + */ 566 + #define unsafe_get_user(x, ptr, label) \ 567 + do { \ 568 + __label__ local_label; \ 569 + arch_unsafe_get_user(x, ptr, local_label); \ 570 + if (0) { \ 571 + local_label: \ 572 + goto label; \ 573 + } \ 574 + } while (0) 575 + 576 + #define unsafe_put_user(x, ptr, label) \ 577 + do { \ 578 + __label__ local_label; \ 579 + arch_unsafe_put_user(x, ptr, local_label); \ 580 + if (0) { \ 581 + local_label: \ 582 + goto label; \ 583 + } \ 584 + } while (0) 585 + #endif /* arch_unsafe_get_user */ 586 + 587 + #else /* user_access_begin */ 581 588 #define user_access_begin(ptr,len) access_ok(ptr, len) 582 589 #define user_access_end() do { } while (0) 583 590 #define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0) ··· 622 559 #define unsafe_copy_from_user(d,s,l,e) unsafe_op_wrap(__copy_from_user(d,s,l),e) 623 560 static inline unsigned long user_access_save(void) { return 0UL; } 624 561 static inline void user_access_restore(unsigned long flags) { } 625 - #endif 562 + #endif /* !user_access_begin */ 563 + 626 564 #ifndef user_write_access_begin 627 565 #define user_write_access_begin user_access_begin 628 566 #define user_write_access_end user_access_end