| Message ID | 20221114104632.3547266-1-zhengyejian1@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 067df9e0ad48a97382ab2179bbe773a13a846028 |
| Headers | show |
| Series | tracing: Fix potential null-pointer-access of entry in list 'tr->err_log' | expand |
On Mon, 14 Nov 2022 18:46:32 +0800 Zheng Yejian <zhengyejian1@huawei.com> wrote: > Entries in list 'tr->err_log' will be reused after entry number > exceed TRACING_LOG_ERRS_MAX. > > The cmd string of the to be reused entry will be freed first then > allocated a new one. If the allocation failed, then the entry will > still be in list 'tr->err_log' but its 'cmd' field is set to be NULL, > later access of 'cmd' is risky. > > Currently above problem can cause the loss of 'cmd' information of first > entry in 'tr->err_log'. When execute `cat /sys/kernel/tracing/error_log`, > reproduce logs like: > [ 37.495100] trace_kprobe: error: Maxactive is not for kprobe(null) ^ > [ 38.412517] trace_kprobe: error: Maxactive is not for kprobe > Command: p4:myprobe2 do_sys_openat2 > ^ This looks good to me. Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> BTW, I'm interested in how did you reproduce it. Did you inject a memory allocation failure to reproduce it? Thank you, > > Fixes: 1581a884b7ca ("tracing: Remove size restriction on tracing_log_err cmd strings") > Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> > --- > kernel/trace/trace.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index 47a44b055a1d..5ae776598106 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -7802,6 +7802,7 @@ static struct tracing_log_err *get_tracing_log_err(struct trace_array *tr, > int len) > { > struct tracing_log_err *err; > + char *cmd; > > if (tr->n_err_log_entries < TRACING_LOG_ERRS_MAX) { > err = alloc_tracing_log_err(len); > @@ -7810,12 +7811,12 @@ static struct tracing_log_err *get_tracing_log_err(struct trace_array *tr, > > return err; > } > - > + cmd = kzalloc(len, GFP_KERNEL); > + if (!cmd) > + return ERR_PTR(-ENOMEM); > err = list_first_entry(&tr->err_log, struct tracing_log_err, list); > kfree(err->cmd); > - err->cmd = kzalloc(len, GFP_KERNEL); > - if (!err->cmd) > - return ERR_PTR(-ENOMEM); > + err->cmd = cmd; > list_del(&err->list); > > return err; > -- > 2.25.1 >
On 2022/11/14 20:46, Masami Hiramatsu (Google) wrote: > On Mon, 14 Nov 2022 18:46:32 +0800 > Zheng Yejian <zhengyejian1@huawei.com> wrote: > >> Entries in list 'tr->err_log' will be reused after entry number >> exceed TRACING_LOG_ERRS_MAX. >> >> The cmd string of the to be reused entry will be freed first then >> allocated a new one. If the allocation failed, then the entry will >> still be in list 'tr->err_log' but its 'cmd' field is set to be NULL, >> later access of 'cmd' is risky. >> >> Currently above problem can cause the loss of 'cmd' information of first >> entry in 'tr->err_log'. When execute `cat /sys/kernel/tracing/error_log`, >> reproduce logs like: >> [ 37.495100] trace_kprobe: error: Maxactive is not for kprobe(null) ^ >> [ 38.412517] trace_kprobe: error: Maxactive is not for kprobe >> Command: p4:myprobe2 do_sys_openat2 >> ^ > > This looks good to me. > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thanks! > > BTW, I'm interested in how did you reproduce it. Did you inject a memory > allocation failure to reproduce it? Yes. I happended to find this problem when I was looking at the code, then I inject an allocation failure to proof it. > > Thank you, > >> >> Fixes: 1581a884b7ca ("tracing: Remove size restriction on tracing_log_err cmd strings") >> Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> >> --- >> kernel/trace/trace.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >> index 47a44b055a1d..5ae776598106 100644 >> --- a/kernel/trace/trace.c >> +++ b/kernel/trace/trace.c >> @@ -7802,6 +7802,7 @@ static struct tracing_log_err *get_tracing_log_err(struct trace_array *tr, >> int len) >> { >> struct tracing_log_err *err; >> + char *cmd; >> >> if (tr->n_err_log_entries < TRACING_LOG_ERRS_MAX) { >> err = alloc_tracing_log_err(len); >> @@ -7810,12 +7811,12 @@ static struct tracing_log_err *get_tracing_log_err(struct trace_array *tr, >> >> return err; >> } >> - >> + cmd = kzalloc(len, GFP_KERNEL); >> + if (!cmd) >> + return ERR_PTR(-ENOMEM); >> err = list_first_entry(&tr->err_log, struct tracing_log_err, list); >> kfree(err->cmd); >> - err->cmd = kzalloc(len, GFP_KERNEL); >> - if (!err->cmd) >> - return ERR_PTR(-ENOMEM); >> + err->cmd = cmd; >> list_del(&err->list); >> >> return err; >> -- >> 2.25.1 >> > >
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 47a44b055a1d..5ae776598106 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -7802,6 +7802,7 @@ static struct tracing_log_err *get_tracing_log_err(struct trace_array *tr, int len) { struct tracing_log_err *err; + char *cmd; if (tr->n_err_log_entries < TRACING_LOG_ERRS_MAX) { err = alloc_tracing_log_err(len); @@ -7810,12 +7811,12 @@ static struct tracing_log_err *get_tracing_log_err(struct trace_array *tr, return err; } - + cmd = kzalloc(len, GFP_KERNEL); + if (!cmd) + return ERR_PTR(-ENOMEM); err = list_first_entry(&tr->err_log, struct tracing_log_err, list); kfree(err->cmd); - err->cmd = kzalloc(len, GFP_KERNEL); - if (!err->cmd) - return ERR_PTR(-ENOMEM); + err->cmd = cmd; list_del(&err->list); return err;
Entries in list 'tr->err_log' will be reused after entry number exceed TRACING_LOG_ERRS_MAX. The cmd string of the to be reused entry will be freed first then allocated a new one. If the allocation failed, then the entry will still be in list 'tr->err_log' but its 'cmd' field is set to be NULL, later access of 'cmd' is risky. Currently above problem can cause the loss of 'cmd' information of first entry in 'tr->err_log'. When execute `cat /sys/kernel/tracing/error_log`, reproduce logs like: [ 37.495100] trace_kprobe: error: Maxactive is not for kprobe(null) ^ [ 38.412517] trace_kprobe: error: Maxactive is not for kprobe Command: p4:myprobe2 do_sys_openat2 ^ Fixes: 1581a884b7ca ("tracing: Remove size restriction on tracing_log_err cmd strings") Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com> --- kernel/trace/trace.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)