linux-kernel - Re: [PATCH] tracing: Verify event formats that have "%*p.."

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <856e7f9f-2a7f-4b94-b6b9-c1f7151a1f7b@oracle.com>
Date: Thu, 27 Mar 2025 11:32:23 -0700
From: Libo Chen <libo.chen@...cle.com>
To: Steven Rostedt <rostedt@...dmis.org>, LKML
 <linux-kernel@...r.kernel.org>,
        Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>
Cc: Masami Hiramatsu <mhiramat@...nel.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH] tracing: Verify event formats that have "%*p.."



On 3/27/25 08:49, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
> 
> The trace event verifier checks the formats of trace events to make sure
> that they do not point at memory that is not in the trace event itself or
> in data that will never be freed. If an event references data that was
> allocated when the event triggered and that same data is freed before the
> event is read, then the kernel can crash by reading freed memory.
> 
> The verifier runs at boot up (or module load) and scans the print formats
> of the events and checks their arguments to make sure that dereferenced
> pointers are safe. If the format uses "%*p.." the verifier will ignore it,
> and that could be dangerous. Cover this case as well.
> 
> Also add to the sample code a use case of "%*pbl".
> 
> Link: https://urldefense.com/v3/__https://lore.kernel.org/all/bcba4d76-2c3f-4d11-baf0-02905db953dd@oracle.com/__;!!ACWV5N9M2RV99hQ!LTIVO1_O9wY2hBAnpNF5zcB1EFlC77zOnu4oVcM4DoD77p5ZO_m1LFZdPVJPj4spzye4JntXHOqOVxirPQ$ 
> 
Thanks Steve, a minor style issue below, otherwise LGTM. I have tested it, it works. 

Reviewed-by: Libo Chen <libo.chen@...cle.com>
Tested-by: Libo Chen <libo.chen@...cle.com>
 

-- Libo

> Reported-by: Libo Chen <libo.chen@...cle.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
>  kernel/trace/trace_events.c                | 7 +++++++
>  samples/trace_events/trace-events-sample.h | 8 ++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 7b3ef1d26167..34e7b4af9f88 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -470,6 +470,7 @@ static void test_event_printk(struct trace_event_call *call)
>  			case '%':
>  				continue;
>  			case 'p':
> + do_pointer:
>  				/* Find dereferencing fields */
>  				switch (fmt[i + 1]) {
>  				case 'B': case 'R': case 'r':
> @@ -498,6 +499,12 @@ static void test_event_printk(struct trace_event_call *call)
>  						continue;
>  					if (fmt[i + j] == '*') {
>  						star = true;
> +						/* Handle %*pbl case */
> +						if (!j && fmt[i + 1] == 'p') {
> +							arg++;
> +							i++;
> +							goto do_pointer;
> +						}
>  						continue;
>  					}
>  					if ((fmt[i + j] == 's')) {
> diff --git a/samples/trace_events/trace-events-sample.h b/samples/trace_events/trace-events-sample.h
> index 999f78d380ae..0622c9e3f2be 100644
> --- a/samples/trace_events/trace-events-sample.h
> +++ b/samples/trace_events/trace-events-sample.h
> @@ -319,7 +319,8 @@ TRACE_EVENT(foo_bar,
>  		__assign_cpumask(cpum, cpumask_bits(mask));
>  	),
>  
> -	TP_printk("foo %s %d %s %s %s %s %s %s (%s) (%s) %s", __entry->foo, __entry->bar,
> +	TP_printk("foo %s %d %s %s %s %s %s %s (%s) (%s) %s [%d] %*pbl",
> +		  __entry->foo, __entry->bar,
>  
>  /*
>   * Notice here the use of some helper functions. This includes:
> @@ -370,7 +371,10 @@ TRACE_EVENT(foo_bar,
>  
>  		  __get_str(str), __get_str(lstr),
>  		  __get_bitmask(cpus), __get_cpumask(cpum),
> -		  __get_str(vstr))
> +		  __get_str(vstr),
> +	         __get_dynamic_array_len(cpus),
> +	         __get_dynamic_array_len(cpus),
> +	         __get_dynamic_array(cpus))

__get_dynamic_array_len() calls don't seem to be aligned with the ones above

>  );
>  
>  /*