All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tracing: Add length protection to histogram string copies
@ 2021-11-14 18:28 Steven Rostedt
  2021-11-15  0:04 ` Masami Hiramatsu
  0 siblings, 1 reply; 2+ messages in thread
From: Steven Rostedt @ 2021-11-14 18:28 UTC (permalink / raw)
  To: LKML
  Cc: Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu,
	Linus Torvalds

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The string copies to the histogram storage has a max size of 256 bytes
(defined by MAX_FILTER_STR_VAL). Only the string size of the event field
needs to be copied to the event storage, but no more than what is in the
event storage. Although nothing should be bigger than 256 bytes, there's
no protection against overwriting of the storage if one day there is.

Copy no more than the destination size, and enforce it.

Also had to turn MAX_FILTER_STR_VAL into an unsigned int, to keep the
min() comparison of the string sizes of comparable types.

Link: https://lore.kernel.org/all/CAHk-=wjREUihCGrtRBwfX47y_KrLCGjiq3t6QtoNJpmVrAEb1w@mail.gmail.com/

Cc: Tom Zanussi <zanussi@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Fixes: 63f84ae6b82b ("tracing/histogram: Do not copy the fixed-size char array field over the field size")
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/trace_events.h     | 2 +-
 kernel/trace/trace_events_hist.c | 9 +++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 50453b287615..2d167ac3452c 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -673,7 +673,7 @@ struct trace_event_file {
 
 #define PERF_MAX_TRACE_SIZE	8192
 
-#define MAX_FILTER_STR_VAL	256	/* Should handle KSYM_SYMBOL_LEN */
+#define MAX_FILTER_STR_VAL	256U	/* Should handle KSYM_SYMBOL_LEN */
 
 enum event_trigger_type {
 	ETT_NONE		= (0),
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 1475d7347fe0..34afcaebd0e5 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -3026,8 +3026,10 @@ static inline void __update_field_vars(struct tracing_map_elt *elt,
 		if (val->flags & HIST_FIELD_FL_STRING) {
 			char *str = elt_data->field_var_str[j++];
 			char *val_str = (char *)(uintptr_t)var_val;
+			unsigned int size;
 
-			strscpy(str, val_str, val->size);
+			size = min(val->size, STR_VAR_LEN_MAX);
+			strscpy(str, val_str, size);
 			var_val = (u64)(uintptr_t)str;
 		}
 		tracing_map_set_var(elt, var_idx, var_val);
@@ -4914,6 +4916,7 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data,
 			if (hist_field->flags & HIST_FIELD_FL_STRING) {
 				unsigned int str_start, var_str_idx, idx;
 				char *str, *val_str;
+				unsigned int size;
 
 				str_start = hist_data->n_field_var_str +
 					hist_data->n_save_var_str;
@@ -4922,7 +4925,9 @@ 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, hist_field->size);
+
+				size = min(hist_field->size, STR_VAR_LEN_MAX);
+				strscpy(str, val_str, size);
 
 				hist_val = (u64)(uintptr_t)str;
 			}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] tracing: Add length protection to histogram string copies
  2021-11-14 18:28 [PATCH] tracing: Add length protection to histogram string copies Steven Rostedt
@ 2021-11-15  0:04 ` Masami Hiramatsu
  0 siblings, 0 replies; 2+ messages in thread
From: Masami Hiramatsu @ 2021-11-15  0:04 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: LKML, Ingo Molnar, Andrew Morton, Tom Zanussi, Masami Hiramatsu,
	Linus Torvalds

On Sun, 14 Nov 2021 13:28:34 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The string copies to the histogram storage has a max size of 256 bytes
> (defined by MAX_FILTER_STR_VAL). Only the string size of the event field
> needs to be copied to the event storage, but no more than what is in the
> event storage. Although nothing should be bigger than 256 bytes, there's
> no protection against overwriting of the storage if one day there is.
> 
> Copy no more than the destination size, and enforce it.
> 
> Also had to turn MAX_FILTER_STR_VAL into an unsigned int, to keep the
> min() comparison of the string sizes of comparable types.
> 
> Link: https://lore.kernel.org/all/CAHk-=wjREUihCGrtRBwfX47y_KrLCGjiq3t6QtoNJpmVrAEb1w@mail.gmail.com/
> 

Oops, right. The field size can be defined over the
MAX_FILTER_STR_VAL. This looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>


> Cc: Tom Zanussi <zanussi@kernel.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Fixes: 63f84ae6b82b ("tracing/histogram: Do not copy the fixed-size char array field over the field size")
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  include/linux/trace_events.h     | 2 +-
>  kernel/trace/trace_events_hist.c | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
> index 50453b287615..2d167ac3452c 100644
> --- a/include/linux/trace_events.h
> +++ b/include/linux/trace_events.h
> @@ -673,7 +673,7 @@ struct trace_event_file {
>  
>  #define PERF_MAX_TRACE_SIZE	8192
>  
> -#define MAX_FILTER_STR_VAL	256	/* Should handle KSYM_SYMBOL_LEN */
> +#define MAX_FILTER_STR_VAL	256U	/* Should handle KSYM_SYMBOL_LEN */
>  
>  enum event_trigger_type {
>  	ETT_NONE		= (0),
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 1475d7347fe0..34afcaebd0e5 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -3026,8 +3026,10 @@ static inline void __update_field_vars(struct tracing_map_elt *elt,
>  		if (val->flags & HIST_FIELD_FL_STRING) {
>  			char *str = elt_data->field_var_str[j++];
>  			char *val_str = (char *)(uintptr_t)var_val;
> +			unsigned int size;
>  
> -			strscpy(str, val_str, val->size);
> +			size = min(val->size, STR_VAR_LEN_MAX);
> +			strscpy(str, val_str, size);
>  			var_val = (u64)(uintptr_t)str;
>  		}
>  		tracing_map_set_var(elt, var_idx, var_val);
> @@ -4914,6 +4916,7 @@ static void hist_trigger_elt_update(struct hist_trigger_data *hist_data,
>  			if (hist_field->flags & HIST_FIELD_FL_STRING) {
>  				unsigned int str_start, var_str_idx, idx;
>  				char *str, *val_str;
> +				unsigned int size;
>  
>  				str_start = hist_data->n_field_var_str +
>  					hist_data->n_save_var_str;
> @@ -4922,7 +4925,9 @@ 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, hist_field->size);
> +
> +				size = min(hist_field->size, STR_VAR_LEN_MAX);
> +				strscpy(str, val_str, size);
>  
>  				hist_val = (u64)(uintptr_t)str;
>  			}
> -- 
> 2.31.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-11-15  0:05 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-14 18:28 [PATCH] tracing: Add length protection to histogram string copies Steven Rostedt
2021-11-15  0:04 ` Masami Hiramatsu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.