| Message ID | 1440791509-5450-2-git-send-email-ast@plumgrid.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 28 Aug 2015 12:51:48 -0700 Alexei Starovoitov <ast@plumgrid.com> wrote: > EXPORT_SYMBOL(strncpy_from_user); > + > +/** > + * strncpy_from_unsafe: - Copy a NUL terminated string from unsafe address. > + * @dst: Destination address, in kernel space. This buffer must be at > + * least @count bytes long. > + * @src: Unsafe address. > + * @count: Maximum number of bytes to copy, including the trailing NUL. > + * > + * Copies a NUL-terminated string from unsafe address to kernel buffer. > + * > + * On success, returns the length of the string (not including the trailing > + * NUL). I think it includes the NUL. > + * > + * If access fails, returns -EFAULT (some data may have been copied). > + * > + * If @count is smaller than the length of the string, copies @count bytes > + * and returns @count. > + */ > +long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count) > +{ > + mm_segment_t old_fs = get_fs(); > + const void *src = unsafe_addr; src = unsafe_addr = 0x100; *unsafe_addr = "1\0"; > + long ret; > + > + if (unlikely(count <= 0)) > + return 0; > + > + set_fs(KERNEL_DS); > + pagefault_disable(); > + > + do { > + ret = __copy_from_user_inatomic(dst++, > + (const void __user __force *)src++, 1); First loop: dst[-1] = '1' src = 0x101 Second loop: dst[-1] = '\0' src = 0x102 > + } while (dst[-1] && ret == 0 && src - unsafe_addr < count); > + > + dst[-1] = '\0'; > + pagefault_enable(); > + set_fs(old_fs); > + > + return ret < 0 ? ret : src - unsafe_addr; src - unsafe_addr = 0x102 - 0x100 = 2 Included the NUL. -- Steve > +} -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/28/15 2:48 PM, Steven Rostedt wrote: >> * On success, returns the length of the string (not including the trailing >> >+ * NUL). > I think it includes the NUL. oops. yes. that was a copy paste from strncpy_from_user comment. trace_kprobe usage wants NUL to be counted, so I intended to have it counted, but that brings the question what should be the semantics. Should it be similar to strncpy_from_user (not counting NUL) or similar to strlen_user (counts NUL) ? imo counting NUL makes a little bit more sense, since when a user says strncpy_from_unsafe(..., ..., 32) and it returns 32 as the whole buffer was filled, it looks cleaner. Thoughts? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 28 Aug 2015 15:08:35 -0700 Alexei Starovoitov <ast@plumgrid.com> wrote: > On 8/28/15 2:48 PM, Steven Rostedt wrote: > >> * On success, returns the length of the string (not including the trailing > >> >+ * NUL). > > I think it includes the NUL. > > oops. yes. that was a copy paste from strncpy_from_user comment. > trace_kprobe usage wants NUL to be counted, so I intended to have it > counted, but that brings the question what should be the semantics. > Should it be similar to strncpy_from_user (not counting NUL) or > similar to strlen_user (counts NUL) ? > imo counting NUL makes a little bit more sense, since when a user says > strncpy_from_unsafe(..., ..., 32) > and it returns 32 as the whole buffer was filled, it looks cleaner. > Thoughts? > I personally prefer counting the NUL. I've had issues in the past with the strncpy_from_user() not counting it :-p -- Steve -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/28/15 3:11 PM, Steven Rostedt wrote: > I personally prefer counting the NUL. I've had issues in the past with > the strncpy_from_user() not counting it :-p agreed. will respin with comment fixed. thank you much for review! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index ae572c138607..d6f2c2c5b043 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -129,4 +129,6 @@ extern long __probe_kernel_read(void *dst, const void *src, size_t size); extern long notrace probe_kernel_write(void *dst, const void *src, size_t size); extern long notrace __probe_kernel_write(void *dst, const void *src, size_t size); +extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count); + #endif /* __LINUX_UACCESS_H__ */ diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index b7d0cdd9906c..c9956440d0e6 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -165,11 +165,9 @@ DEFINE_BASIC_FETCH_FUNCS(memory) static void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs, void *addr, void *dest) { - long ret; int maxlen = get_rloc_len(*(u32 *)dest); u8 *dst = get_rloc_data(dest); - u8 *src = addr; - mm_segment_t old_fs = get_fs(); + long ret; if (!maxlen) return; @@ -178,23 +176,13 @@ static void FETCH_FUNC_NAME(memory, string)(struct pt_regs *regs, * Try to get string again, since the string can be changed while * probing. */ - set_fs(KERNEL_DS); - pagefault_disable(); - - do - ret = __copy_from_user_inatomic(dst++, src++, 1); - while (dst[-1] && ret == 0 && src - (u8 *)addr < maxlen); - - dst[-1] = '\0'; - pagefault_enable(); - set_fs(old_fs); + ret = strncpy_from_unsafe(dst, addr, maxlen); if (ret < 0) { /* Failed to fetch string */ - ((u8 *)get_rloc_data(dest))[0] = '\0'; + dst[0] = '\0'; *(u32 *)dest = make_data_rloc(0, get_rloc_offs(*(u32 *)dest)); } else { - *(u32 *)dest = make_data_rloc(src - (u8 *)addr, - get_rloc_offs(*(u32 *)dest)); + *(u32 *)dest = make_data_rloc(ret, get_rloc_offs(*(u32 *)dest)); } } NOKPROBE_SYMBOL(FETCH_FUNC_NAME(memory, string)); diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index e0af6ff73d14..89dce35ee6cf 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -112,3 +112,44 @@ long strncpy_from_user(char *dst, const char __user *src, long count) return -EFAULT; } EXPORT_SYMBOL(strncpy_from_user); + +/** + * strncpy_from_unsafe: - Copy a NUL terminated string from unsafe address. + * @dst: Destination address, in kernel space. This buffer must be at + * least @count bytes long. + * @src: Unsafe address. + * @count: Maximum number of bytes to copy, including the trailing NUL. + * + * Copies a NUL-terminated string from unsafe address to kernel buffer. + * + * On success, returns the length of the string (not including the trailing + * NUL). + * + * If access fails, returns -EFAULT (some data may have been copied). + * + * If @count is smaller than the length of the string, copies @count bytes + * and returns @count. + */ +long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count) +{ + mm_segment_t old_fs = get_fs(); + const void *src = unsafe_addr; + long ret; + + if (unlikely(count <= 0)) + return 0; + + set_fs(KERNEL_DS); + pagefault_disable(); + + do { + ret = __copy_from_user_inatomic(dst++, + (const void __user __force *)src++, 1); + } while (dst[-1] && ret == 0 && src - unsafe_addr < count); + + dst[-1] = '\0'; + pagefault_enable(); + set_fs(old_fs); + + return ret < 0 ? ret : src - unsafe_addr; +}
generalize FETCH_FUNC_NAME(memory, string) into strncpy_from_unsafe() and fix sparse warnings that were present in original implementation. Signed-off-by: Alexei Starovoitov <ast@plumgrid.com> --- include/linux/uaccess.h | 2 ++ kernel/trace/trace_kprobe.c | 20 ++++---------------- lib/strncpy_from_user.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 16 deletions(-)