Jann Horn <ja...@google.com> writes: > On Thu, Feb 7, 2019 at 10:22 AM Christophe Leroy > <christophe.le...@c-s.fr> wrote: >> In powerpc code, there are several places implementing safe >> access to user data. This is sometimes implemented using >> probe_kernel_address() with additional access_ok() verification, >> sometimes with get_user() enclosed in a pagefault_disable()/enable() >> pair, etc. : >> show_user_instructions() >> bad_stack_expansion() >> p9_hmi_special_emu() >> fsl_pci_mcheck_exception() >> read_user_stack_64() >> read_user_stack_32() on PPC64 >> read_user_stack_32() on PPC32 >> power_pmu_bhrb_to() >> >> In the same spirit as probe_kernel_read(), this patch adds >> probe_user_read(). >> >> probe_user_read() does the same as probe_kernel_read() but >> first checks that it is really a user address. >> >> The patch defines this function as a static inline so the "size" >> variable can be examined for const-ness by the check_object_size() >> in __copy_from_user_inatomic() >> >> Signed-off-by: Christophe Leroy <christophe.le...@c-s.fr> > > > >> --- >> v3: Moved 'Returns:" comment after description. >> Explained in the commit log why the function is defined static inline >> >> v2: Added "Returns:" comment and removed probe_user_address() >> >> include/linux/uaccess.h | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h >> index 37b226e8df13..ef99edd63da3 100644 >> --- a/include/linux/uaccess.h >> +++ b/include/linux/uaccess.h >> @@ -263,6 +263,40 @@ extern long strncpy_from_unsafe(char *dst, const void >> *unsafe_addr, long count); >> #define probe_kernel_address(addr, retval) \ >> probe_kernel_read(&retval, addr, sizeof(retval)) >> >> +/** >> + * probe_user_read(): safely attempt to read from a user location >> + * @dst: pointer to the buffer that shall take the data >> + * @src: address to read from >> + * @size: size of the data chunk >> + * >> + * Safely read from address @src to the buffer at @dst. If a kernel fault >> + * happens, handle that and return -EFAULT. >> + * >> + * We ensure that the copy_from_user is executed in atomic context so that >> + * do_page_fault() doesn't attempt to take mmap_sem. This makes >> + * probe_user_read() suitable for use within regions where the caller >> + * already holds mmap_sem, or other locks which nest inside mmap_sem. >> + * >> + * Returns: 0 on success, -EFAULT on error. >> + */ >> + >> +#ifndef probe_user_read >> +static __always_inline long probe_user_read(void *dst, const void __user >> *src, >> + size_t size) >> +{ >> + long ret; >> + >> + if (!access_ok(src, size)) >> + return -EFAULT; > > If this happens in code that's running with KERNEL_DS, the access_ok() > is a no-op. If this helper is only intended for accessing real > userspace memory, it would be more robust to add > set_fs(USER_DS)/set_fs(oldfs) around this thing. Looking at the > functions you're referring to in the commit message, e.g. > show_user_instructions() does an explicit `__access_ok(pc, > NR_INSN_TO_PRINT * sizeof(int), USER_DS)` to get the same effect.
Yeah I raised the same question up thread. I think we're both right :) - it should explicitly set USER_DS. There's precedent for that in the code you mentioned and also in the perf code, eg: 88b0193d9418 ("perf/callchain: Force USER_DS when invoking perf_callchain_user()") cheers