lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 27 May 2020 19:26:30 -0700 From: Yonghong Song <yhs@...com> To: Andrew Morton <akpm@...ux-foundation.org>, Christoph Hellwig <hch@....de> CC: <x86@...nel.org>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Masami Hiramatsu <mhiramat@...nel.org>, Linus Torvalds <torvalds@...ux-foundation.org>, <linux-parisc@...r.kernel.org>, <linux-um@...ts.infradead.org>, <netdev@...r.kernel.org>, <bpf@...r.kernel.org>, <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org> Subject: Re: [PATCH 12/23] bpf: handle the compat string in bpf_trace_copy_string better On 5/27/20 7:04 PM, Andrew Morton wrote: > On Thu, 21 May 2020 17:22:50 +0200 Christoph Hellwig <hch@....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@...ux-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@....de> > Cc: Alexei Starovoitov <ast@...nel.org> > Cc: Daniel Borkmann <daniel@...earbox.net> > Cc: "H. Peter Anvin" <hpa@...or.com> > Cc: Ingo Molnar <mingo@...e.hu> > Cc: Masami Hiramatsu <mhiramat@...nel.org> > Cc: Thomas Gleixner <tglx@...utronix.de> > Signed-off-by: Andrew Morton <akpm@...ux-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]; > _ >
Powered by blists - more mailing lists