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-=wh5jE5ARarmYNdL4sja36_e-mnejv3zRMC62Jzn-a3omw@mail.gmail.com>
Date: Sun, 15 Dec 2024 09:23:18 -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 Sun, 15 Dec 2024 at 02:04, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> What va_list bug? That logic didn't break.

That logic HAS NEVER EVER WORKED RIGHT.

You are literally mis-using va_list. The code is *wrong*. It depends
on the exact calling convention of varargs, and just happens to work
on many platforms.

And even when the calling conventions happened to match, it only
happens to work because of how vsnprintf() treats its incoming
argument.

So my complaint is literally that the code you are fixing is
unfixable. It's a "it happens to work" situation (except when it
didn't), not a "this code is right" situation.

And dammit, you must have known this code was garbage, because when I
do a "git blame", I see your name on that test_can_verify_check()
function.

Basically, you are walking the va_list in invalid ways, because you
want to use vsnprintf() in invalid ways. As part of that walk, you are
(a) fundamentally mis-using va_list and (b) modifying the format
string as you go along (both in the sense of actually changing the
format string, but also switching that format pointer around).

My issue with this is that (b) was buggy, but it was a bug that came
from a much more *fundamental* bug - the misuse of vsnprintf.   Which
is why I'm saying "rip it out". Because your fix is fixing symptoms.
It's not fixing the fact that you are doing things that are wrong.

Looking up more, I also think the whole "replace "%p" with "%px" is
broken and wrong.

Isn't every single case of '%p' in this context from a TP_printk() situation?

IOW, instead of dynamically creating a temporary buffer and adding
that 'x' by hand, why wasn't that just a 'sed' script and done
statically?

In fact, wouldn't *most* of the sanity checking be possible to just do
entirely statically instead of at runtime?

Yeah, yeah, there's more than a few '%p' users, but

     git grep 'TP_printk.*%p[^A-Za-z]' | wc

shows that it's manageable. That probably misses some multi-line
cases, but still - doing this kind of "rewrite constant string at
runtime because we didn't do it statically" seems *wrong*.

And in this case, that wrongness was literally the cause of the bug.

                Linus