Writing a hist trigger whose value or variable carries a modifier that is
not allowed there leaks the fields that were built for it.
__create_val_field() takes the field from parse_expr() and stores it in
hist_data->fields[] only after the modifier checks have run:
hist_field = parse_expr(hist_data, file, field_str, flags, var_name,
&n_subexprs);
...
if (hist_field->flags & HIST_FIELD_FL_VAR) {
if (hist_field->flags & (...))
goto err;
} else {
if (hist_field->flags & (...))
goto err;
}
hist_data->fields[val_idx] = hist_field;
Both checks jump past that store, and the err label returns without
freeing anything. The error unwinds to create_hist_data(), which calls
destroy_hist_data() -> destroy_hist_fields(), and that reaches a field
only by walking fields[]. A field that never got there is unreachable.
commit e0213434fe3e ("tracing: Do not let histogram values have some
modifiers") set ret to -EINVAL and fell through to the store, which left
the field owned by fields[] and freed along with the rest of hist_data.
Splitting the check into a value case and a variable case replaced that
fall-through with a goto that skips it.
With CONFIG_DEBUG_KMEMLEAK, 200 writes of
# echo 'hist:keys=prev_pid:vals=next_pid.log2' > \
events/sched/sched_switch/trigger
each correctly rejected with -EINVAL, leave 332 unreferenced objects
(63744 bytes) reported at create_hist_field(); 200 install and remove
cycles of a valid trigger leave none. A '.log2' field is two
allocations, since create_hist_field() puts the plain field in
operands[0] of the log2 field, and both are reported.
Use destroy_hist_field() rather than __destroy_hist_field() so that
operands[0] is freed as well. It returns early for HIST_FIELD_FL_VAR_REF,
which is what an operand owned by hist_data->var_refs[] needs; the
rejected field itself is never a var ref, because a var ref never carries
a modifier flag.
Fixes: e30fbc618e97 ("tracing/histograms: Allow variables to have some
modifiers")
Cc: [email protected]
Signed-off-by: Donggeun Yoo <[email protected]>
---
Tested under QEMU x86_64 on 1fc5a74b108f, both kernels built from the
same config:
vals=next_pid.log2, 200 writes 332 objects / 63744 bytes -> 0
x=next_pid.log2, 200 writes 471 objects / 61544 bytes -> 0
valid trigger, 200 install/remove 0 -> 0
All 400 writes are still rejected with -EINVAL after the change. A
CONFIG_KASAN build reports nothing on the same runs. tools/testing/
selftests/ftrace trigger tests are unchanged: 32 pass, 3 fail, 2
unresolved before and after, with the failures also present on an
unpatched kernel.
kernel/trace/trace_events_hist.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 963e0d6b61fd..b65d79d6e132 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4331,6 +4331,7 @@ static int __create_val_field(struct hist_trigger_data
*hist_data,
return ret;
err:
hist_err(file->tr, HIST_ERR_BAD_FIELD_MODIFIER, errpos(field_str));
+ destroy_hist_field(hist_field, 0);
return -EINVAL;
}
--
2.53.0
