Skip to content

Commit

Permalink
um: fix _nofault accesses
Browse files Browse the repository at this point in the history
Nathan reported [1] that when built with clang, the um kernel
crashes pretty much immediately. This turned out to be an issue
with the inline assembly I had added, when clang used %rax/%eax
for both operands. Reorder it so current->thread.segv_continue
is written first, and then the lifetime of _faulted won't have
overlap with the lifetime of segv_continue.

In the email thread Benjamin also pointed out that current->mm
is only NULL for true kernel tasks, but we could do this for a
userspace task, so the current->thread.segv_continue logic must
be lifted out of the mm==NULL check.

Finally, while looking at this, put a barrier() so the NULL
assignment to thread.segv_continue cannot be reorder before
the possibly faulting operation.

Reported-by: Nathan Chancellor <nathan@kernel.org>
Closes: https://lore.kernel.org/r/20250402221254.GA384@ax162 [1]
Fixes: d1d7f01 ("um: mark rodata read-only and implement _nofault accesses")
Tested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
  • Loading branch information
Johannes Berg committed May 5, 2025
1 parent 92a09c4 commit 68025ad
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 15 deletions.
2 changes: 2 additions & 0 deletions arch/um/include/asm/uaccess.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ do { \
goto err_label; \
} \
*((type *)dst) = get_unaligned((type *)(src)); \
barrier(); \
current->thread.segv_continue = NULL; \
} while (0)

Expand All @@ -66,6 +67,7 @@ do { \
if (__faulted) \
goto err_label; \
put_unaligned(*((type *)src), (type *)(dst)); \
barrier(); \
current->thread.segv_continue = NULL; \
} while (0)

Expand Down
26 changes: 13 additions & 13 deletions arch/um/kernel/trap.c
Original file line number Diff line number Diff line change
Expand Up @@ -225,20 +225,20 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
panic("Failed to sync kernel TLBs: %d", err);
goto out;
}
else if (current->mm == NULL) {
if (current->pagefault_disabled) {
if (!mc) {
show_regs(container_of(regs, struct pt_regs, regs));
panic("Segfault with pagefaults disabled but no mcontext");
}
if (!current->thread.segv_continue) {
show_regs(container_of(regs, struct pt_regs, regs));
panic("Segfault without recovery target");
}
mc_set_rip(mc, current->thread.segv_continue);
current->thread.segv_continue = NULL;
goto out;
else if (current->pagefault_disabled) {
if (!mc) {
show_regs(container_of(regs, struct pt_regs, regs));
panic("Segfault with pagefaults disabled but no mcontext");
}
if (!current->thread.segv_continue) {
show_regs(container_of(regs, struct pt_regs, regs));
panic("Segfault without recovery target");
}
mc_set_rip(mc, current->thread.segv_continue);
current->thread.segv_continue = NULL;
goto out;
}
else if (current->mm == NULL) {
show_regs(container_of(regs, struct pt_regs, regs));
panic("Segfault with no mm");
}
Expand Down
2 changes: 1 addition & 1 deletion arch/x86/um/shared/sysdep/faultinfo_32.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ struct faultinfo {

#define ___backtrack_faulted(_faulted) \
asm volatile ( \
"mov $0, %0\n" \
"movl $__get_kernel_nofault_faulted_%=,%1\n" \
"mov $0, %0\n" \
"jmp _end_%=\n" \
"__get_kernel_nofault_faulted_%=:\n" \
"mov $1, %0;" \
Expand Down
2 changes: 1 addition & 1 deletion arch/x86/um/shared/sysdep/faultinfo_64.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ struct faultinfo {

#define ___backtrack_faulted(_faulted) \
asm volatile ( \
"mov $0, %0\n" \
"movq $__get_kernel_nofault_faulted_%=,%1\n" \
"mov $0, %0\n" \
"jmp _end_%=\n" \
"__get_kernel_nofault_faulted_%=:\n" \
"mov $1, %0;" \
Expand Down

0 comments on commit 68025ad

Please sign in to comment.