linux-kernel - Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs

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]

Date:   Thu, 16 Mar 2017 10:40:24 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...capital.net>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH 4/5 v2] ftrace/x86_32: Clean up ftrace_regs_caller

On Thu, Mar 16, 2017 at 10:20 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> When ftrace_regs_caller was created, it was designed to preserve flags as
> much as possible as it needed to act just like a breakpoint triggered on the
> same location. But the design is over complicated as it treated all
> operations as modifying flags. But push, mov and lea do not modify flags.
> This means the code can become more simplified by allowing flags to be
> stored further down.

It still looks overly complicated to me.

The snippet below is the patch without the "-" lines, so it's the end result:

>  ENTRY(ftrace_regs_caller)
>         /*
>          * i386 does not save SS and ESP when coming from kernel.
>          * Instead, to get sp, &regs->sp is used (see ptrace.h).
>          * Unfortunately, that means eflags must be at the same location
>          * as the current return ip is. We move the return ip into the
> +        * regs->ip location, and move flags into the return ip location.
> +         */
> +       pushl   $__KERNEL_CS
> +       pushl   4(%esp)                         /* Save the return ip */
> +
> +       /* temporarily save flags in the orig_ax location */
> +       pushf
>
>         pushl   %gs
>         pushl   %fs
>         pushl   %es
>         pushl   %ds
>         pushl   %eax
> +
> +       /* move flags into the location of where the return ip was */
> +       movl    5*4(%esp), %eax
> +       movl    $0, 5*4(%esp)                   /* Load 0 into orig_ax */
> +       movl    %eax, 8*4(%esp)                 /* Load flags in return ip */

Why do you do that silly "temporarily save flags" thing?

Why not just push $0 there?

Afaik, the sequence could/should be:

        pushl   $__KERNEL_CS
        pushl   4(%esp)         /* Save the return ip */
        pushl   $0
        pushl   %gs
        pushl   %fs
        pushl   %es
        pushl   %ds
        pushl   %eax

        /* Fix up eflags now that we have a scratch register */
        pushfl
        popl    %eax
        movl    %eax,8(%rsp)

Or something. There's no point in the unnecessary "shuffle values back
and forth with odd stack offsets".

           Linus