On Tue, 7 Apr 2026 14:09:10 +0800 Pengpeng Hou <[email protected]> wrote:
> expr_str() allocates a fixed MAX_FILTER_STR_VAL buffer and then builds > expression names with a series of raw strcat() appends. Nested operands, > constants and field flags can push the rendered string past that fixed > limit before the name is attached to the hist field. > > Convert the construction helpers to explicit bounded appends and > propagate failures back to the expression parser when the rendered name > would exceed MAX_FILTER_STR_VAL. > > Signed-off-by: Pengpeng Hou <[email protected]> > --- > kernel/trace/trace_events_hist.c | 101 +++++++++++++++++++++++-------- > 1 file changed, 76 insertions(+), 25 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c > b/kernel/trace/trace_events_hist.c > index 73ea180cad55..caaa262360d2 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1738,85 +1738,121 @@ static const char *get_hist_field_flags(struct > hist_field *hist_field) > return flags_str; > } > > -static void expr_field_str(struct hist_field *field, char *expr) > +static bool expr_append(char *expr, size_t *len, const char *str) > { > - if (field->flags & HIST_FIELD_FL_VAR_REF) > - strcat(expr, "$"); > - else if (field->flags & HIST_FIELD_FL_CONST) { > + size_t str_len = strlen(str); > + > + if (*len + str_len >= MAX_FILTER_STR_VAL) > + return false; > + > + memcpy(expr + *len, str, str_len + 1); > + *len += str_len; > + return true; > +} > + This looks like a better job for seq_buf. > +static bool expr_field_str(struct hist_field *field, char *expr, size_t *len) > +{ struct seq_buf s; seq_buf_init(&s, expr, MAX_FILTER_STR_VAL); > + if (field->flags & HIST_FIELD_FL_VAR_REF) { > + if (!expr_append(expr, len, "$")) > + return false; seq_buf_putc(&s, '$'); > + } else if (field->flags & HIST_FIELD_FL_CONST) { > char str[HIST_CONST_DIGITS_MAX]; > + int ret; > + > + ret = snprintf(str, sizeof(str), "%llu", field->constant); > + if (ret >= sizeof(str)) > + return false; > > - snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant); > - strcat(expr, str); seq_buf_printf(&s, "%llu", field->constant); > + if (!expr_append(expr, len, str)) > + return false; > } > > - strcat(expr, hist_field_name(field, 0)); seq_buf_puts(&s, hist_field_name(field, 0)); > + if (!expr_append(expr, len, hist_field_name(field, 0))) > + return false; > > if (field->flags && !(field->flags & HIST_FIELD_FL_VAR_REF)) { > const char *flags_str = get_hist_field_flags(field); > > if (flags_str) { > - strcat(expr, "."); > - strcat(expr, flags_str); seq_buf_printf(&s, ".%s", flags_str); > + if (!expr_append(expr, len, ".") || > + !expr_append(expr, len, flags_str)) > + return false; > } > } /* Add terminating character */ seq_buf_str(&s); return seq_buf_overflow(&s) ? false : true; > + > + return true; > } > > static char *expr_str(struct hist_field *field, unsigned int level) > { > char *expr; > + size_t len = 0; > This could all be converted too. -- Steve > if (level > 1) > - return NULL; > + return ERR_PTR(-EINVAL); > > expr = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL); > if (!expr) > - return NULL; > + return ERR_PTR(-ENOMEM); > > if (!field->operands[0]) { > - expr_field_str(field, expr); > + if (!expr_field_str(field, expr, &len)) > + goto free; > return expr; > } > > if (field->operator == FIELD_OP_UNARY_MINUS) { > char *subexpr; > > - strcat(expr, "-("); > + if (!expr_append(expr, &len, "-(")) > + goto free; > subexpr = expr_str(field->operands[0], ++level); > if (!subexpr) { > - kfree(expr); > - return NULL; > + goto free; > + } > + if (!expr_append(expr, &len, subexpr) || > + !expr_append(expr, &len, ")")) { > + kfree(subexpr); > + goto free; > } > - strcat(expr, subexpr); > - strcat(expr, ")"); > > kfree(subexpr); > > return expr; > } > > - expr_field_str(field->operands[0], expr); > + if (!expr_field_str(field->operands[0], expr, &len)) > + goto free; > > switch (field->operator) { > case FIELD_OP_MINUS: > - strcat(expr, "-"); > + if (!expr_append(expr, &len, "-")) > + goto free; > break; > case FIELD_OP_PLUS: > - strcat(expr, "+"); > + if (!expr_append(expr, &len, "+")) > + goto free; > break; > case FIELD_OP_DIV: > - strcat(expr, "/"); > + if (!expr_append(expr, &len, "/")) > + goto free; > break; > case FIELD_OP_MULT: > - strcat(expr, "*"); > + if (!expr_append(expr, &len, "*")) > + goto free; > break; > default: > - kfree(expr); > - return NULL; > + goto free; > } > > - expr_field_str(field->operands[1], expr); > + if (!expr_field_str(field->operands[1], expr, &len)) > + goto free; > > return expr; > + > +free: > + kfree(expr); > + return ERR_PTR(-E2BIG); > } > > /* > @@ -2630,6 +2666,11 @@ static struct hist_field *parse_unary(struct > hist_trigger_data *hist_data, > expr->is_signed = operand1->is_signed; > expr->operator = FIELD_OP_UNARY_MINUS; > expr->name = expr_str(expr, 0); > + if (IS_ERR(expr->name)) { > + ret = PTR_ERR(expr->name); > + expr->name = NULL; > + goto free; > + } > expr->type = kstrdup_const(operand1->type, GFP_KERNEL); > if (!expr->type) { > ret = -ENOMEM; > @@ -2842,6 +2883,11 @@ static struct hist_field *parse_expr(struct > hist_trigger_data *hist_data, > destroy_hist_field(operand1, 0); > > expr->name = expr_str(expr, 0); > + if (IS_ERR(expr->name)) { > + ret = PTR_ERR(expr->name); > + expr->name = NULL; > + goto free_expr; > + } > } else { > /* The operand sizes should be the same, so just pick one */ > expr->size = operand1->size; > @@ -2855,6 +2901,11 @@ static struct hist_field *parse_expr(struct > hist_trigger_data *hist_data, > } > > expr->name = expr_str(expr, 0); > + if (IS_ERR(expr->name)) { > + ret = PTR_ERR(expr->name); > + expr->name = NULL; > + goto free_expr; > + } > } > > return expr;
