From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH] bpf: fix check of allowed specifiers in bpf_trace_printk Date: Thu, 22 Nov 2018 22:31:00 +0100 Message-ID: <43c54a7b-167a-4585-2147-05ec65e95a4b@iogearbox.net> References: <20181122160030.15954-1-m@lambda.lt> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: ast@kernel.org To: Martynas Pumputis , netdev@vger.kernel.org Return-path: Received: from www62.your-server.de ([213.133.104.62]:48442 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732619AbeKWIMO (ORCPT ); Fri, 23 Nov 2018 03:12:14 -0500 In-Reply-To: <20181122160030.15954-1-m@lambda.lt> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: Hi Martynas, On 11/22/2018 05:00 PM, Martynas Pumputis wrote: > A format string consisting of "%p" or "%s" followed by an invalid > specifier (e.g. "%p%\n" or "%s%") could pass the check which > would make format_decode (lib/vsprintf.c) to warn. > > Reported-by: syzbot+1ec5c5ec949c4adaa0c4@syzkaller.appspotmail.com > Signed-off-by: Martynas Pumputis > --- > kernel/trace/bpf_trace.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 08fcfe440c63..9ab05736e1a1 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -225,6 +225,8 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1, > (void *) (long) unsafe_addr, > sizeof(buf)); > } > + if (fmt[i] == '%') > + i--; > continue; > } Thanks for the fix! Could we simplify the logic a bit to avoid having to navigate i back and forth which got us in trouble in the first place? Like below (untested) perhaps? diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 08fcfe4..ff83b8c 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -196,11 +196,13 @@ BPF_CALL_5(bpf_trace_printk, char *, fmt, u32, fmt_size, u64, arg1, i++; } else if (fmt[i] == 'p' || fmt[i] == 's') { mod[fmt_cnt]++; - i++; - if (!isspace(fmt[i]) && !ispunct(fmt[i]) && fmt[i] != 0) + /* Disallow any further format extensions. */ + if (fmt[i + 1] != 0 && + !isspace(fmt[i + 1]) && + !ispunct(fmt[i + 1])) return -EINVAL; fmt_cnt++; - if (fmt[i - 1] == 's') { + if (fmt[i] == 's') { if (str_seen) /* allow only one '%s' per fmt string */ return -EINVAL; Thanks, Daniel