linux-kernel - Re: [GIT PULL] tracing: Prevent trace_marker being bigger than unsigned short

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] [day] [month] [year] [list]

Date: Mon, 4 Mar 2024 20:20:09 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Steven Rostedt <rostedt@...dmis.org>,
 Linus Torvalds <torvalds@...ux-foundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
 Masami Hiramatsu <mhiramat@...nel.org>, Sachin Sant <sachinp@...ux.ibm.com>
Subject: Re: [GIT PULL] tracing: Prevent trace_marker being bigger than
 unsigned short

On 2024-03-04 19:43, Steven Rostedt wrote:
> On Mon, 4 Mar 2024 16:17:15 -0800
> Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
>> On Mon, 4 Mar 2024 at 15:50, Steven Rostedt <rostedt@...dmis.org> wrote:
>>>
>>> But this still isn't fixing anything. It's just adding a limit.
>>
>> Limiting things to a common maximum size is a good thing. The kernel
>> limits much  more important things for very good reasons.
>>
>> The kernel really shouldn't have big strings. EVER.  And it literally
>> shows in our kernel infrastructure. It showed in that vsnprintf
>> precision thing. It shows in our implementation choices, where we tend
>> to have simplistic implementations because doing things a byte at a
>> time is simple and cheap when the strings are limited in size (and we
>> don't want fancy and can't use vector state anyway).
>>
>> If something as core as a pathname can be limited to 4kB, then
>> something as unimportant as a trace string had better be limited too.
>> Because we simply DO NOT WANT to have to deal with longer strings in
>> the kernel.
>>
> 
> So I made three patches that do basically what you want. And as a bonus,
> it's not really an arbitrary limit but based on trace_seq size.
> 
> The first patch will be removing the precision check, as that's not needed.
> 
> The second patch is to remove the dependency between trace_seq and
> PAGE_SIZE, as its size really can just be 8K for all architectures. Which
> has the side effect of limiting the size of trace_marker, as its size is
> limited by the trace_seq size.
> 
> Finally, because the trace_seq defines the max output that a trace_event
> can write (for all its fields), the extra data of a print event could
> possibly overflow that, which will cause the event not to print, and just
> an "OVERFLOW" output would show in the trace buffer. So I used the
> TRACE_SEQ_SIZE / 2 as the max size that trace_marker can read, which
> happens to be 4K.

Steven, see my other reply. This is backwards.

You can leave the trace_seq as is if you want. It's the trace marking
input size that should be #define to 4kB, not defined as
half-the-size-of-an-internal-buffer-that-happens-to-be-8k.

Then add a BUILD_BUG_ON() in the output code to make sure the output
buffer is always large enough.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com