| Message ID | 20200521152301.2587579-13-hch@lst.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/23] maccess: unexport probe_kernel_write and probe_user_write | expand |
On Thu, May 21, 2020 at 8:24 AM Christoph Hellwig <hch@lst.de> wrote: > > User the proper helper for kernel or userspace addresses based on > TASK_SIZE instead of the dangerous strncpy_from_unsafe function. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Acked-by: Andrii Nakryiko <andriin@fb.com> > kernel/trace/bpf_trace.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > [...]
On Thu, 21 May 2020 17:22:50 +0200 Christoph Hellwig <hch@lst.de> wrote: > User the proper helper for kernel or userspace addresses based on > TASK_SIZE instead of the dangerous strncpy_from_unsafe function. > > ... > > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -331,8 +331,11 @@ static void bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype, > switch (fmt_ptype) { > case 's': > #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE > - strncpy_from_unsafe(buf, unsafe_ptr, bufsz); > - break; > + if ((unsigned long)unsafe_ptr < TASK_SIZE) { > + strncpy_from_user_nofault(buf, user_ptr, bufsz); > + break; > + } > + fallthrough; > #endif > case 'k': > strncpy_from_kernel_nofault(buf, unsafe_ptr, bufsz); Another user of strncpy_from_unsafe() has popped up in linux-next's bpf. I did the below, but didn't try very hard - it's probably wrong if CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE=n? Anyway, please take a look at all the bpf_trace.c changes in linux-next. From: Andrew Morton <akpm@linux-foundation.org> Subject: bpf:bpf_seq_printf(): handle potentially unsafe format string better User the proper helper for kernel or userspace addresses based on TASK_SIZE instead of the dangerous strncpy_from_unsafe function. Cc: Christoph Hellwig <hch@lst.de> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Daniel Borkmann <daniel@iogearbox.net> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Ingo Molnar <mingo@elte.hu> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- kernel/trace/bpf_trace.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) --- a/kernel/trace/bpf_trace.c~xxx +++ a/kernel/trace/bpf_trace.c @@ -588,15 +588,22 @@ BPF_CALL_5(bpf_seq_printf, struct seq_fi } if (fmt[i] == 's') { + void *unsafe_ptr; + /* try our best to copy */ if (memcpy_cnt >= MAX_SEQ_PRINTF_MAX_MEMCPY) { err = -E2BIG; goto out; } - err = strncpy_from_unsafe(bufs->buf[memcpy_cnt], - (void *) (long) args[fmt_cnt], - MAX_SEQ_PRINTF_STR_LEN); + unsafe_ptr = (void *)(long)args[fmt_cnt]; + if ((unsigned long)unsafe_ptr < TASK_SIZE) { + err = strncpy_from_user_nofault( + bufs->buf[memcpy_cnt], unsafe_ptr, + MAX_SEQ_PRINTF_STR_LEN); + } else { + err = -EFAULT; + } if (err < 0) bufs->buf[memcpy_cnt][0] = '\0'; params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt];
On 5/27/20 7:04 PM, Andrew Morton wrote: > On Thu, 21 May 2020 17:22:50 +0200 Christoph Hellwig <hch@lst.de> wrote: > >> User the proper helper for kernel or userspace addresses based on >> TASK_SIZE instead of the dangerous strncpy_from_unsafe function. >> >> ... >> >> --- a/kernel/trace/bpf_trace.c >> +++ b/kernel/trace/bpf_trace.c >> @@ -331,8 +331,11 @@ static void bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype, >> switch (fmt_ptype) { >> case 's': >> #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE >> - strncpy_from_unsafe(buf, unsafe_ptr, bufsz); >> - break; >> + if ((unsigned long)unsafe_ptr < TASK_SIZE) { >> + strncpy_from_user_nofault(buf, user_ptr, bufsz); >> + break; >> + } >> + fallthrough; >> #endif >> case 'k': >> strncpy_from_kernel_nofault(buf, unsafe_ptr, bufsz); > > Another user of strncpy_from_unsafe() has popped up in linux-next's > bpf. I did the below, but didn't try very hard - it's probably wrong > if CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE=n? > > Anyway, please take a look at all the bpf_trace.c changes in > linux-next. > > > From: Andrew Morton <akpm@linux-foundation.org> > Subject: bpf:bpf_seq_printf(): handle potentially unsafe format string better > > User the proper helper for kernel or userspace addresses based on > TASK_SIZE instead of the dangerous strncpy_from_unsafe function. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Alexei Starovoitov <ast@kernel.org> > Cc: Daniel Borkmann <daniel@iogearbox.net> > Cc: "H. Peter Anvin" <hpa@zytor.com> > Cc: Ingo Molnar <mingo@elte.hu> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > > kernel/trace/bpf_trace.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > --- a/kernel/trace/bpf_trace.c~xxx > +++ a/kernel/trace/bpf_trace.c > @@ -588,15 +588,22 @@ BPF_CALL_5(bpf_seq_printf, struct seq_fi > } > > if (fmt[i] == 's') { > + void *unsafe_ptr; > + > /* try our best to copy */ > if (memcpy_cnt >= MAX_SEQ_PRINTF_MAX_MEMCPY) { > err = -E2BIG; > goto out; > } > > - err = strncpy_from_unsafe(bufs->buf[memcpy_cnt], > - (void *) (long) args[fmt_cnt], > - MAX_SEQ_PRINTF_STR_LEN); > + unsafe_ptr = (void *)(long)args[fmt_cnt]; > + if ((unsigned long)unsafe_ptr < TASK_SIZE) { > + err = strncpy_from_user_nofault( > + bufs->buf[memcpy_cnt], unsafe_ptr, > + MAX_SEQ_PRINTF_STR_LEN); > + } else { > + err = -EFAULT; > + } This probably not right. The pointer stored at args[fmt_cnt] is a kernel pointer, but it could be an invalid address and we do not want to fault. Not sure whether it exists or not, we should use strncpy_from_kernel_nofault()? > if (err < 0) > bufs->buf[memcpy_cnt][0] = '\0'; > params[fmt_cnt] = (u64)(long)bufs->buf[memcpy_cnt]; > _ >
On Wed, May 27, 2020 at 07:26:30PM -0700, Yonghong Song wrote: >> --- a/kernel/trace/bpf_trace.c~xxx >> +++ a/kernel/trace/bpf_trace.c >> @@ -588,15 +588,22 @@ BPF_CALL_5(bpf_seq_printf, struct seq_fi >> } >> if (fmt[i] == 's') { >> + void *unsafe_ptr; >> + >> /* try our best to copy */ >> if (memcpy_cnt >= MAX_SEQ_PRINTF_MAX_MEMCPY) { >> err = -E2BIG; >> goto out; >> } >> - err = strncpy_from_unsafe(bufs->buf[memcpy_cnt], >> - (void *) (long) args[fmt_cnt], >> - MAX_SEQ_PRINTF_STR_LEN); >> + unsafe_ptr = (void *)(long)args[fmt_cnt]; >> + if ((unsigned long)unsafe_ptr < TASK_SIZE) { >> + err = strncpy_from_user_nofault( >> + bufs->buf[memcpy_cnt], unsafe_ptr, >> + MAX_SEQ_PRINTF_STR_LEN); >> + } else { >> + err = -EFAULT; >> + } > > This probably not right. > The pointer stored at args[fmt_cnt] is a kernel pointer, > but it could be an invalid address and we do not want to fault. > Not sure whether it exists or not, we should use > strncpy_from_kernel_nofault()? If you know it is a kernel pointer with this series it should be strncpy_from_kernel_nofault. But even before the series it should have been strncpy_from_unsafe_strict.
On 5/27/20 9:39 PM, Christoph Hellwig wrote: > On Wed, May 27, 2020 at 07:26:30PM -0700, Yonghong Song wrote: >>> --- a/kernel/trace/bpf_trace.c~xxx >>> +++ a/kernel/trace/bpf_trace.c >>> @@ -588,15 +588,22 @@ BPF_CALL_5(bpf_seq_printf, struct seq_fi >>> } >>> if (fmt[i] == 's') { >>> + void *unsafe_ptr; >>> + >>> /* try our best to copy */ >>> if (memcpy_cnt >= MAX_SEQ_PRINTF_MAX_MEMCPY) { >>> err = -E2BIG; >>> goto out; >>> } >>> - err = strncpy_from_unsafe(bufs->buf[memcpy_cnt], >>> - (void *) (long) args[fmt_cnt], >>> - MAX_SEQ_PRINTF_STR_LEN); >>> + unsafe_ptr = (void *)(long)args[fmt_cnt]; >>> + if ((unsigned long)unsafe_ptr < TASK_SIZE) { >>> + err = strncpy_from_user_nofault( >>> + bufs->buf[memcpy_cnt], unsafe_ptr, >>> + MAX_SEQ_PRINTF_STR_LEN); >>> + } else { >>> + err = -EFAULT; >>> + } >> >> This probably not right. >> The pointer stored at args[fmt_cnt] is a kernel pointer, >> but it could be an invalid address and we do not want to fault. >> Not sure whether it exists or not, we should use >> strncpy_from_kernel_nofault()? > > If you know it is a kernel pointer with this series it should be > strncpy_from_kernel_nofault. But even before the series it should have > been strncpy_from_unsafe_strict. The use of strncpy_from_unsafe() mimics old bpf_trace_printk() implementation which just changed to _strict version: https://lkml.org/lkml/2020/5/18/1309 Agreed that we should change to strncpy_from_unsafe_strict(). I can submit a patch for this. Thanks!
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 9d4080590f711..737d739230a6b 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -331,8 +331,11 @@ static void bpf_trace_copy_string(char *buf, void *unsafe_ptr, char fmt_ptype, switch (fmt_ptype) { case 's': #ifdef CONFIG_ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE - strncpy_from_unsafe(buf, unsafe_ptr, bufsz); - break; + if ((unsigned long)unsafe_ptr < TASK_SIZE) { + strncpy_from_user_nofault(buf, user_ptr, bufsz); + break; + } + fallthrough; #endif case 'k': strncpy_from_kernel_nofault(buf, unsafe_ptr, bufsz);
User the proper helper for kernel or userspace addresses based on TASK_SIZE instead of the dangerous strncpy_from_unsafe function. Signed-off-by: Christoph Hellwig <hch@lst.de> --- kernel/trace/bpf_trace.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)