| Message ID | 39fb6c5a191025378676492e140dc012915ecaeb.1547652372.git.christophe.leroy@c-s.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] mm: add probe_user_read() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 40 lines checked |
Christophe Leroy <christophe.leroy@c-s.fr> writes: > 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.leroy@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; > + I wonder if we should explicitly switch to USER_DS here? That would be sort of unusual, but the whole reason for this helper existing is to make sure we safely read from user memory and not accidentally from kernel. cheers > + if (!access_ok(src, size)) > + return -EFAULT; > + > + pagefault_disable(); > + ret = __copy_from_user_inatomic(dst, src, size); > + pagefault_enable(); > + > + return ret ? -EFAULT : 0; > +} > +#endif > + > #ifndef user_access_begin > #define user_access_begin(ptr,len) access_ok(ptr, len) > #define user_access_end() do { } while (0) > -- > 2.13.3
Hi, Christophe. On Wed, Jan 16, 2019 at 04:59:27PM +0000, Christophe Leroy 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.leroy@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; Hopefully, there is still time for a minor comment. Do we need to differentiate the returned error here, e.g.: return -EACCES? I wonder if there will be situations where callers need to know why probe_user_read() failed. Besides that: Acked-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com> > + > + pagefault_disable(); > + ret = __copy_from_user_inatomic(dst, src, size); > + pagefault_enable(); > + > + return ret ? -EFAULT : 0; > +} > +#endif > + > #ifndef user_access_begin > #define user_access_begin(ptr,len) access_ok(ptr, len) > #define user_access_end() do { } while (0) > -- > 2.13.3 > -- Murilo
Murilo Opsfelder Araujo <muriloo@linux.ibm.com> writes: >> 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; > > Hopefully, there is still time for a minor comment. > > Do we need to differentiate the returned error here, e.g.: return > -EACCES? > > I wonder if there will be situations where callers need to know why > probe_user_read() failed. It's pretty standard to return EFAULT when an access_ok() check fails, so I think using EFAULT here is the safest option. If we used EACCES we'd need to be more careful about converting code to use this helper, as doing so would potentially cause the error value to change, which in some cases is not OK. cheers
On Thu, Feb 7, 2019 at 10:22 AM Christophe Leroy <christophe.leroy@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.leroy@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. (However, __access_ok() looks like it's horribly broken on x86 from what I can tell, because it's going to use the generic version that always returns 1...) > + pagefault_disable(); > + ret = __copy_from_user_inatomic(dst, src, size); > + pagefault_enable(); > + > + return ret ? -EFAULT : 0; > +} > +#endif > + > #ifndef user_access_begin > #define user_access_begin(ptr,len) access_ok(ptr, len) > #define user_access_end() do { } while (0) > -- > 2.13.3 > >
On Wed, Jan 16, 2019 at 04:59:27PM +0000, Christophe Leroy wrote: > 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() The correct spelling is 'Return:', not 'Returns:': Return values ~~~~~~~~~~~~ The return value, if any, should be described in a dedicated section named ``Return``.
Jann Horn <jannh@google.com> writes: > On Thu, Feb 7, 2019 at 10:22 AM Christophe Leroy > <christophe.leroy@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.leroy@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
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; + + pagefault_disable(); + ret = __copy_from_user_inatomic(dst, src, size); + pagefault_enable(); + + return ret ? -EFAULT : 0; +} +#endif + #ifndef user_access_begin #define user_access_begin(ptr,len) access_ok(ptr, len) #define user_access_end() do { } while (0)
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.leroy@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(+)