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+1ec5c5ec949c4adaa...@syzkaller.appspotmail.com > Signed-off-by: Martynas Pumputis <m...@lambda.lt> > --- > 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