[RFC][PATCH] printf: Harden accessing pointer dereference in vsprintf()

[RFC][PATCH] printf: Harden accessing pointer dereference in vsprintf()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

For extra safety from crashing the kernel, add a
copy_from_kernel_nofault() in check_pointer_msg(). If it fails to read the
memory, then return "(efault)".

This isn't full proof, as the length of the pointer being read could
possibly go into bad memory, but this should catch the majority of errors.

Linus had suggested adding this kind of check[1]. This is a bit different
than Linus's solution as it utilizes copy_from_kernel_nofault() and doesn't
require calls to pagefault_disable() and extra labels.

[1] https://lore.kernel.org/all/CAHk-=wh3cUC2a=yJv42HTjDLCp6VM+GTky+q65vV_Q33BeoxAg@mail.gmail.com/

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 lib/vsprintf.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 9d3dac38a3f4..1a533f1174f0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -695,12 +695,18 @@ static char *error_string(char *buf, char *end, const char *s,
  */
 static const char *check_pointer_msg(const void *ptr)
 {
+	char ch;
+
 	if (!ptr)
 		return "(null)";
 
 	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
 		return "(efault)";
 
+	/* Just test a single byte */
+	if (copy_from_kernel_nofault(&ch, ptr, 1) < 0)
+		return "(efault)";
+
 	return NULL;
 }
 
-- 
2.45.2

Re: [RFC][PATCH] printf: Harden accessing pointer dereference in vsprintf()

[Linus Torvalds]

On Mon, 6 Jan 2025 at 14:25, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Linus had suggested adding this kind of check[1]. This is a bit different
> than Linus's solution as it utilizes copy_from_kernel_nofault() and doesn't
> require calls to pagefault_disable() and extra labels.

Yeah, and it generates horrendous code as a result.

               Linus

Re: [RFC][PATCH] printf: Harden accessing pointer dereference in vsprintf()

[Kees Cook]

On Mon, Jan 06, 2025 at 05:27:22PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> 
> For extra safety from crashing the kernel, add a
> copy_from_kernel_nofault() in check_pointer_msg(). If it fails to read the
> memory, then return "(efault)".
> 
> This isn't full proof, as the length of the pointer being read could
> possibly go into bad memory, but this should catch the majority of errors.
> 
> Linus had suggested adding this kind of check[1]. This is a bit different
> than Linus's solution as it utilizes copy_from_kernel_nofault() and doesn't
> require calls to pagefault_disable() and extra labels.
> 
> [1] https://lore.kernel.org/all/CAHk-=wh3cUC2a=yJv42HTjDLCp6VM+GTky+q65vV_Q33BeoxAg@mail.gmail.com/
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Seems reasonable to me.

Reviewed-by: Kees Cook <kees@kernel.org>

-Kees

-- 
Kees Cook

Re: [RFC][PATCH] printf: Harden accessing pointer dereference in vsprintf()

[Steven Rostedt]

On Mon, 6 Jan 2025 15:29:01 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Mon, 6 Jan 2025 at 14:25, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Linus had suggested adding this kind of check[1]. This is a bit different
> > than Linus's solution as it utilizes copy_from_kernel_nofault() and doesn't
> > require calls to pagefault_disable() and extra labels.  
> 
> Yeah, and it generates horrendous code as a result.
> 

I guess the question is do we prefer horrendous code generated or what we see?

vsprintf() is used in critical paths, but I wonder how much these two
versions actually make a difference in performance? If they do, then yeah,
the open coded version would be better.

-- Steve

Re: [RFC][PATCH] printf: Harden accessing pointer dereference in vsprintf()

[Linus Torvalds]

On Mon, 6 Jan 2025 at 18:28, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> vsprintf() is used in critical paths, but I wonder how much these two
> versions actually make a difference in performance?

We've had people writing patches to avoid vsnprintf because it's *so*
critical for /proc etc.

And we had actual bugs as a result, eg things like

   f9ed1f7c2e26 ("genirq/proc: Use seq_put_decimal_ull_width() for
decimal values")

then causing

   9d9f204bdf72 ("genirq/proc: Add missing space separator back")

so bad performance results in actual BUGGY CODE.

It's one of the reasons I was looking at vsnprintf even before the
whole tracing discussion: some of the vsnprintf overhead was actually
disgusting. And almost all of it was just stupid code, allegedly for
simplicity. Causing things like pointless stack frame setup, bad
calling conventions etc.

So my vsnprintf branch was literally written with me looking at the
resulting assembler too, because when core code works badly, the end
result is badness.

In random non-core code, I want people to write it clearly and
maintainably. I very much do not want random drivers doing
micro-optimizations etc.

End result: I don't want people to say "oh, vsnprintf does badly at
handling '%s' so I'm going to use some special string-printing thing",
the way we had that seq_put_decimal_ull_width() chaos.

And yes, I saw that very error_string() function inlined several times
when I was looking at code generation. gcc and clang did things
differently, I forget which way it went, but there was a reason I sent
out the test patch that I had *checked* made code generation almost
identical. The "fetch a byte" was still fetch a simple byte fetch.

Iirc the main difference was that yes, we had to do that annoying
"pagefault_disable/enable()" and that generated something like five
instructions just to get 'current' and increment a variable.. But it
didn't cause extra stack frames or function calls. It's been about a
month, I forget the exact details.

(And yes, I even considered turning pagefault_disable/enable there
into a preempt_count() thing instead. We do better at those percpu
variables than at per-thread ones. We used to do it that way, but we
made pagefault_disable its own counter back ten years ago exactly
*because* people wanted to have a separate counter, and sadly that
does mean that it's now a few instructions).

               Linus

Re: [RFC][PATCH] printf: Harden accessing pointer dereference in vsprintf()

[Sergey Senozhatsky]

On (25/01/06 17:27), Steven Rostedt wrote:
> @@ -695,12 +695,18 @@ static char *error_string(char *buf, char *end, const char *s,
>   */
>  static const char *check_pointer_msg(const void *ptr)
>  {
> +	char ch;
> +
>  	if (!ptr)
>  		return "(null)";
>  
>  	if ((unsigned long)ptr < PAGE_SIZE || IS_ERR_VALUE(ptr))
>  		return "(efault)";
>  
> +	/* Just test a single byte */
> +	if (copy_from_kernel_nofault(&ch, ptr, 1) < 0)
> +		return "(efault)";
> +

I might be wrong, but I recall that copy_from_kernel() (previously known
as probe_kernel_read()) did cause boot regressions in the past.  E.g.
Petr's commit 2ac5a3bf7042 ("vsprintf: Do not break early boot with
probing addresses").