linux-kernel - Re: [GIT PULL] ftrace: Fixes for v6.13

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: <CAHk-=wh3uOnqnZPpR0PeLZZtyWbZLboZ7cHLCKRWsocvs9Y7hQ@mail.gmail.com>
Date: Sat, 14 Dec 2024 21:19:01 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Mark Rutland <mark.rutland@....com>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, 
	Al Viro <viro@...iv.linux.org.uk>, Michal Simek <monstr@...str.eu>
Subject: Re: [GIT PULL] ftrace: Fixes for v6.13

On Sat, 14 Dec 2024 at 20:38, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> So what are you objecting to?

I'm objecting to "we wrote bad code, so let's hack it up some more".

> The current change, or the code that's already been there?

Both.

The current change looked so random that it made me go "WTF?" and then
I looked at the code it's changing.

The code is literally making assumptions about how va_list arguments
even work. It calls trace_seq_printf() and expects that to keep the
"va_list" argument in sync as it walks the argument list.

The code even seems to *know* how hacky and wrong it is, because it
does a RUNTIME CHECK for this in test_can_verify(). But that really is
just testing one random implementation. The whole approach
fundamnetally doesn't work on some architectures at all, and on others
it would be broken by seq_buf_vprintf() (or vsprintf) doing a
va_copy() and using an internal version or any number of other things.

So the code IS WRONG.

It's wrong at a fundamental level. The thing where it modifies
iter->fmt in place is just a detail in the bigger picture of that
function being completely broken.

It's literally hacking together a really *bad* version of KASAN
checking, re-implementing 'vsnprintf()' *badly* by trying to use the
real vsnprintf() for the heavy lifting and literally hacking around
corner cases. And when I say that the "va_list" handling is buggy, I'm
*not* asking you to try to fix it. The va_list bug is just another
symptom of the deeper problem in that thing.

This is why I compared it to the tracefs situation. It has exactly the
same pattern: fundamentally misusing core kernel infrastructure,
hacking around it to get something that "works", but that is wrong on
a fundamental level, and then adding more hacks on top when some new
issue rears its ugly head.

I absolutely believe that coimmit ff54c8dc3f7a fixes some behavior.
But I'm looking at it and going "this code is not salvageable". All
you do is add more bandaids on top.

Do it right. And honestly, "right" in this case is almost certainly
"get rid of trace_check_vprintf() entirely".

The fact that you basically disable the thing already with a static
branch when 'va_list" doesn't work the way you want is a BIG sign.

Just disable it unconditionally.

              Linus