linux-kernel - Re: [PATCH 2/2] tracing: Add a verifier to check string pointers for trace events

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: <YLrRUmwjjaozIt4K@google.com>
Date:   Sat, 5 Jun 2021 01:20:18 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jacob Wen <jian.w.wen@...cle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH 2/2] tracing: Add a verifier to check string pointers for
 trace events

+Paolo

On Fri, Feb 26, 2021, Steven Rostedt wrote:
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index f5e8e39d6f57..ff08c04de6cb 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3551,6 +3551,154 @@ static char *trace_iter_expand_format(struct trace_iterator *iter)
>  	return tmp;
>  }
>  
> +/* Returns true if the string is safe to dereference from an event */
> +static bool trace_safe_str(struct trace_iterator *iter, const char *str)
> +{
> +	unsigned long addr = (unsigned long)str;
> +	struct trace_event *trace_event;
> +	struct trace_event_call *event;
> +
> +	/* OK if part of the event data */
> +	if ((addr >= (unsigned long)iter->ent) &&
> +	    (addr < (unsigned long)iter->ent + iter->ent_size))
> +		return true;
> +
> +	/* OK if part of the temp seq buffer */
> +	if ((addr >= (unsigned long)iter->tmp_seq.buffer) &&
> +	    (addr < (unsigned long)iter->tmp_seq.buffer + PAGE_SIZE))
> +		return true;
> +
> +	/* Core rodata can not be freed */
> +	if (is_kernel_rodata(addr))
> +		return true;
> +
> +	/*
> +	 * Now this could be a module event, referencing core module
> +	 * data, which is OK.
> +	 */

Question on this: is it safe for a module to provide a string from its core data
when invoking a tracepoint that is defined and exported by a different module or
by the kernel proper?

If that behavior is safe, then this will generate false positives on those use
cases.  And AFAICT, it's not possible to fully fix the issue, at least not
without plumbing in more crud.  E.g. if the tracepoint is built into the kernel,
event->mod will be NULL and so even a hack-a-fix like walking source_list isn't
an option (I used source_list to verify the address is indeed in the invoking
module).

E.g. in KVM, all tracepoints are defined and exported by the common 'kvm' module
(which can also be built-in), and used by 'kvm' and the vendor modules,
'kvm_intel' and 'kvm_amd'.  One tracepoint in particular takes a hardcoded string
and does a direct fast assign to and from a 'const char *'.  This triggers the
WARN and displays [UNSAFE-MEMORY] because the string is in 'kvm_intel', not the
tracepoint owner.

The bug is easy to workaround in KVM by converting to the __string() machinery,
but I suspect there will be grumpy users on the horizon when a false positive
breaks a tracepoint in a different module at the worst possible time.

> +	if (!iter->ent)
> +		return false;
> +
> +	trace_event = ftrace_find_event(iter->ent->type);
> +	if (!trace_event)
> +		return false;
> +
> +	event = container_of(trace_event, struct trace_event_call, event);
> +	if (!event->mod)
> +		return false;
> +
> +	/* Would rather have rodata, but this will suffice */
> +	if (within_module_core(addr, event->mod))
> +		return true;
> +
> +	return false;
> +}