[GIT PULL] tracing: Three small fixes for 5.16

[GIT PULL] tracing: Three small fixes for 5.16

[Steven Rostedt]

Linus,

Three tracing fixes:

 - Make local osnoise_instances static

 - Copy just actual size of histogram strings

 - Properly check missing operands in histogram expressions


Please pull the latest trace-v5.16-4 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v5.16-4

Tag SHA1: 1e06c15eb016180a996fd2bd3838fa1ca9ea21d6
Head SHA1: 1cab6bce42e62bba2ff2c2370d139618c1828b42


Daniel Bristot de Oliveira (1):
      tracing/osnoise: Make osnoise_instances static

Kalesh Singh (1):
      tracing/histogram: Fix check for missing operands in an expression

Masami Hiramatsu (1):
      tracing/histogram: Do not copy the fixed-size char array field over the field size

----
 kernel/trace/trace_events_hist.c | 12 +++++++-----
 kernel/trace/trace_osnoise.c     |  3 ++-
 2 files changed, 9 insertions(+), 6 deletions(-)
---------------------------
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 8ff572a31fd3..1475d7347fe0 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1953,9 +1953,10 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
 		if (!hist_field->type)
 			goto free;
 
-		if (field->filter_type == FILTER_STATIC_STRING)
+		if (field->filter_type == FILTER_STATIC_STRING) {
 			hist_field->fn = hist_field_string;
-		else if (field->filter_type == FILTER_DYN_STRING)
+			hist_field->size = field->size;
+		} else if (field->filter_type == FILTER_DYN_STRING)
 			hist_field->fn = hist_field_dynstring;
 		else
 			hist_field->fn = hist_field_pstring;
@@ -2580,7 +2581,8 @@ static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,
 	operand1_str = str;
 	str = sep+1;
 
-	if (!operand1_str || !str)
+	/* Binary operator requires both operands */
+	if (*operand1_str == '\0' || *str == '\0')
 		goto free;
 
 	operand_flags = 0;
@@ -3025,7 +3027,7 @@ static inline void __update_field_vars(struct tracing_map_elt *elt,
 			char *str = elt_data->field_var_str[j++];
 			char *val_str = (char *)(uintptr_t)var_val;
 
-			strscpy(str, val_str, STR_VAR_LEN_MAX);
+			strscpy(str, val_str, val->size);
 			var_val = (u64)(uintptr_t)str;
 		}
 		tracing_map_set_var(elt, var_idx, var_val);
@@ -4920,7 +4922,7 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data,
 
 				str = elt_data->field_var_str[idx];
 				val_str = (char *)(uintptr_t)hist_val;
-				strscpy(str, val_str, STR_VAR_LEN_MAX);
+				strscpy(str, val_str, hist_field->size);
 
 				hist_val = (u64)(uintptr_t)str;
 			}
diff --git a/kernel/trace/trace_osnoise.c b/kernel/trace/trace_osnoise.c
index 3e4a1651e329..7520d43aed55 100644
--- a/kernel/trace/trace_osnoise.c
+++ b/kernel/trace/trace_osnoise.c
@@ -55,7 +55,8 @@ struct osnoise_instance {
 	struct list_head	list;
 	struct trace_array	*tr;
 };
-struct list_head osnoise_instances;
+
+static struct list_head osnoise_instances;
 
 static bool osnoise_has_registered_instances(void)
 {

Re: [GIT PULL] tracing: Three small fixes for 5.16

[Linus Torvalds]

On Sat, Nov 13, 2021 at 5:35 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>  - Copy just actual size of histogram strings

I have pulled this, but I think it's wrong. Or at least it looks
_very_ suspicious.

> -                       strscpy(str, val_str, STR_VAR_LEN_MAX);
> +                       strscpy(str, val_str, val->size);

So now it doesn't overrun the source string any more, but I don't see
what protects it from not overrunning the destination - which is
indeed STR_VAR_LEN_MAX.

Maybe 'val->size' is guaranteed to be sufficiently limited, but that
sure as hell isn't obvious at least lkocally.

So if I read this all right, if you ever have a FILTER_STATIC_STRING
or a FILTER_PTR_STRING that has a field->size that is larger than
STR_VAR_LEN_MAX, you're now screwed.

Maybe that is unrealistic, and never happens. And yes, STR_VAR_LEN_MAX
is fairly big, but I would personally be happier if these kinds of
things checked BOTH the source limits and the destination limits.

And no, we don't have that kind of string helper. I've talked about
this before: people tend to always think that string copies are about
"either source limit or destination limit", but the fact is, both can
exist.

             Linus

Re: [GIT PULL] tracing: Three small fixes for 5.16

[Steven Rostedt]

On Sat, 13 Nov 2021 10:26:29 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Nov 13, 2021 at 5:35 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >  - Copy just actual size of histogram strings  
> 
> I have pulled this, but I think it's wrong. Or at least it looks
> _very_ suspicious.
> 
> > -                       strscpy(str, val_str, STR_VAR_LEN_MAX);
> > +                       strscpy(str, val_str, val->size);  
> 
> So now it doesn't overrun the source string any more, but I don't see
> what protects it from not overrunning the destination - which is
> indeed STR_VAR_LEN_MAX.
> 
> Maybe 'val->size' is guaranteed to be sufficiently limited, but that
> sure as hell isn't obvious at least lkocally.

Ideally this should be checked on the time the histogram is created.
But looking into it, the size could be determined by the size of a
string of an event field. Now in practice, no event field should be
bigger than 256 bytes, but nothing prevents it from happening.

I'll add logic here to include:

	size = min(val->size, STR_VAR_LEN_MAX);

and use that instead.

-- Steve

Re: [GIT PULL] tracing: Three small fixes for 5.16

[pr-tracker-bot]

The pull request you sent on Sat, 13 Nov 2021 08:35:20 -0500:

> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git trace-v5.16-4

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/7c3737c706073792133deeefae33ab17fd06e0c2

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html