linux-kernel - Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

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: <CAMj1kXFjd8AuHaBMLap6RZ18PR9_Cwv2GFbBkswj-e5YpaQFdA@mail.gmail.com>
Date: Wed, 9 Oct 2024 18:13:54 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: Ard Biesheuvel <ardb+git@...gle.com>, linux-trace-kernel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH] x86/ftrace: Don't bother preserving/restoring R10/R11

On Wed, 9 Oct 2024 at 18:02, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Wed,  9 Oct 2024 17:39:01 +0200
> Ard Biesheuvel <ardb+git@...gle.com> wrote:
>
> > diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
> > index 214f30e9f0c0..9e13f9b203d1 100644
> > --- a/arch/x86/kernel/ftrace_64.S
> > +++ b/arch/x86/kernel/ftrace_64.S
> > @@ -216,8 +216,6 @@ SYM_INNER_LABEL(ftrace_regs_caller_op_ptr, SYM_L_GLOBAL)
> >       movq %r14, R14(%rsp)
> >       movq %r13, R13(%rsp)
> >       movq %r12, R12(%rsp)
> > -     movq %r11, R11(%rsp)
> > -     movq %r10, R10(%rsp)
> >       movq %rbx, RBX(%rsp)
>
> This may need to still be saved. Even though they are pretty much useless,
> it can be used by kprobes, and perhaps they want to see what's in R10 or
> R11 from the caller. I'm not sure we want to remove this. Especially since
> we are even saving "flags"!
>

Are you saying kprobes will call this hook from arbitrary other
locations (where R10/R11 might be live), rather than only from the
fentry hook?

> That is, this part is visible by the callbacks, and the ftrace_get_regs()
> is to return a fully populated pt_regs if FTRACE_OPS_FL_SAVE_REGS is set.
>

Fair enough.

> >       /* Copy saved flags */
> >       movq MCOUNT_REG_SIZE(%rsp), %rcx
> > @@ -256,7 +254,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> >       movq R14(%rsp), %r14
> >       movq R13(%rsp), %r13
> >       movq R12(%rsp), %r12
> > -     movq R10(%rsp), %r10
>
> This part of the patch I think is perfectly fine. We haven't been restoring
> R11 for 12 years I'm sure nobody will notice if we don't restore R10.
>

Not sure I follow this reasoning tbh. R10/R11 are guaranteed to be
dead, so I don't see any point in preserving them. But if you do
capture them, shouldn't you at least ensure that the captured values
match the values that the callee will observe? (even though they are
dead and we know it will never look at them)