[PATCH 1/4] tracing/hist: Prevent overflow in histogram expression strings

[Li Qiang]

expr_str() builds a histogram expression in a fixed-size
MAX_FILTER_STR_VAL allocation with unbounded strcat() calls.
Synthetic events permit field names longer than that buffer. Constructing
a trigger expression can therefore write past the allocation.

Build the expression with seq_buf. Propagate construction errors to the
parser and reject strings that overflow the fixed-size buffer with -E2BIG.

Fixes: 100719dcef44 ("tracing: Add simple expression support to hist triggers")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Li Qiang <liqiang01@xxxxxxxxxx>
---
 kernel/trace/trace_events_hist.c | 89 +++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 30 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 82ce492ab268..2b2547546360 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1733,86 +1733,99 @@ 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 void expr_field_str(struct hist_field *field, struct seq_buf *s)
 {
 	if (field->flags & HIST_FIELD_FL_VAR_REF) {
 		if (!field->system)
-			strcat(expr, "$");
+			seq_buf_putc(s, '$');
 	} else if (field->flags & HIST_FIELD_FL_CONST) {
 		char str[HIST_CONST_DIGITS_MAX];
 
 		snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
-		strcat(expr, str);
+		seq_buf_puts(s, str);
 	}
 
-	strcat(expr, hist_field_name(field, 0));
+	seq_buf_puts(s, hist_field_name(field, 0));
 
 	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_putc(s, '.');
+			seq_buf_puts(s, flags_str);
 		}
 	}
 }
 
 static char *expr_str(struct hist_field *field, unsigned int level)
 {
+	struct seq_buf s;
 	char *expr;
+	int ret = 0;
 
 	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);
+
+	seq_buf_init(&s, expr, MAX_FILTER_STR_VAL);
 
 	if (!field->operands[0]) {
-		expr_field_str(field, expr);
-		return expr;
+		expr_field_str(field, &s);
+		goto out;
 	}
 
 	if (field->operator == FIELD_OP_UNARY_MINUS) {
 		char *subexpr;
 
-		strcat(expr, "-(");
+		seq_buf_puts(&s, "-(");
 		subexpr = expr_str(field->operands[0], ++level);
-		if (!subexpr) {
-			kfree(expr);
-			return NULL;
+		if (IS_ERR(subexpr)) {
+			ret = PTR_ERR(subexpr);
+			goto free;
 		}
-		strcat(expr, subexpr);
-		strcat(expr, ")");
+		seq_buf_puts(&s, subexpr);
+		seq_buf_putc(&s, ')');
 
 		kfree(subexpr);
-
-		return expr;
+		goto out;
 	}
 
-	expr_field_str(field->operands[0], expr);
+	expr_field_str(field->operands[0], &s);
 
 	switch (field->operator) {
 	case FIELD_OP_MINUS:
-		strcat(expr, "-");
+		seq_buf_putc(&s, '-');
 		break;
 	case FIELD_OP_PLUS:
-		strcat(expr, "+");
+		seq_buf_putc(&s, '+');
 		break;
 	case FIELD_OP_DIV:
-		strcat(expr, "/");
+		seq_buf_putc(&s, '/');
 		break;
 	case FIELD_OP_MULT:
-		strcat(expr, "*");
+		seq_buf_putc(&s, '*');
 		break;
 	default:
-		kfree(expr);
-		return NULL;
+		ret = -EINVAL;
+		goto free;
 	}
 
-	expr_field_str(field->operands[1], expr);
+	expr_field_str(field->operands[1], &s);
 
+out:
+	seq_buf_str(&s);
+	if (seq_buf_has_overflowed(&s)) {
+		ret = -E2BIG;
+		goto free;
+	}
 	return expr;
+
+free:
+	kfree(expr);
+	return ERR_PTR(ret);
 }
 
 /*
@@ -2556,6 +2569,7 @@ static struct hist_field *parse_unary(struct hist_trigger_data *hist_data,
 				      char *var_name, unsigned int *n_subexprs)
 {
 	struct hist_field *operand1, *expr = NULL;
+	char *expr_name;
 	unsigned long operand_flags;
 	int ret = 0;
 	char *s;
@@ -2625,7 +2639,12 @@ static struct hist_field *parse_unary(struct hist_trigger_data *hist_data,
 	expr->size = operand1->size;
 	expr->is_signed = operand1->is_signed;
 	expr->operator = FIELD_OP_UNARY_MINUS;
-	expr->name = expr_str(expr, 0);
+	expr_name = expr_str(expr, 0);
+	if (IS_ERR(expr_name)) {
+		ret = PTR_ERR(expr_name);
+		goto free;
+	}
+	expr->name = expr_name;
 	expr->type = kstrdup_const(operand1->type, GFP_KERNEL);
 	if (!expr->type) {
 		ret = -ENOMEM;
@@ -2691,7 +2710,7 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 	struct hist_field *var1 = NULL, *var2 = NULL;
 	unsigned long operand_flags, operand2_flags;
 	int field_op, ret = -EINVAL;
-	char *sep, *operand1_str;
+	char *expr_name, *sep, *operand1_str;
 	enum hist_field_fn op_fn;
 	bool combine_consts;
 
@@ -2837,7 +2856,12 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 		destroy_hist_field(operand2, 0);
 		destroy_hist_field(operand1, 0);
 
-		expr->name = expr_str(expr, 0);
+		expr_name = expr_str(expr, 0);
+		if (IS_ERR(expr_name)) {
+			ret = PTR_ERR(expr_name);
+			goto free_expr;
+		}
+		expr->name = expr_name;
 	} else {
 		/* The operand sizes should be the same, so just pick one */
 		expr->size = operand1->size;
@@ -2850,7 +2874,12 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 			goto free_expr;
 		}
 
-		expr->name = expr_str(expr, 0);
+		expr_name = expr_str(expr, 0);
+		if (IS_ERR(expr_name)) {
+			ret = PTR_ERR(expr_name);
+			goto free_expr;
+		}
+		expr->name = expr_name;
 	}
 
 	return expr;
-- 
2.43.0