linux-kernel - Re: [PATCH] x86/asm/entry/32: Slightly better handling of syscall errors in auditing

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: <CALCETrXkd=0VupGkzFnos7t_=p_6U_h1Y7Wy1WVXJAxzznZL-A@mail.gmail.com>
Date:	Fri, 12 Jun 2015 16:24:32 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Denys Vlasenko <dvlasenk@...hat.com>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Borislav Petkov <bp@...en8.de>,
	"H. Peter Anvin" <hpa@...or.com>, Oleg Nesterov <oleg@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Alexei Starovoitov <ast@...mgrid.com>,
	Will Drewry <wad@...omium.org>,
	Kees Cook <keescook@...omium.org>, X86 ML <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/asm/entry/32: Slightly better handling of syscall
 errors in auditing

On Thu, Jun 11, 2015 at 4:47 AM, Denys Vlasenko <dvlasenk@...hat.com> wrote:
> "setbe %al" insn has a register merge stall: it needs to combine
> previous %eax value with new value for the lowest byte.
> Subsequent "movzbl %al,%edi" in turn depends on its completion.
>
> This patch replaces "setbe %al + movzbl %al,%edi" pair of insns
> with "xor %edi,%edi" before the comparison, and conditional "inc %edi".
>
> This results in the same value of %edi as produced by old code,
> but first insn has no dependencies, and we end up with having
> only one insn with deps which executes only if %eax contains error
> return, and both insns are shorter: 2 bytes each versus 3 bytes each.
>
> (The old code was inherited from 32-bit code, where it allowed to avoid
> a conditional jump. Here we have to use a jump anyway).
>
> Signed-off-by: Denys Vlasenko <dvlasenk@...hat.com>
> CC: Linus Torvalds <torvalds@...ux-foundation.org>
> CC: Steven Rostedt <rostedt@...dmis.org>
> CC: Ingo Molnar <mingo@...nel.org>
> CC: Borislav Petkov <bp@...en8.de>
> CC: "H. Peter Anvin" <hpa@...or.com>
> CC: Andy Lutomirski <luto@...capital.net>
> CC: Oleg Nesterov <oleg@...hat.com>
> CC: Frederic Weisbecker <fweisbec@...il.com>
> CC: Alexei Starovoitov <ast@...mgrid.com>
> CC: Will Drewry <wad@...omium.org>
> CC: Kees Cook <keescook@...omium.org>
> CC: x86@...nel.org
> CC: linux-kernel@...r.kernel.org
> ---
>  arch/x86/entry/entry_64_compat.S | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
> index bb187a6..96f33a4 100644
> --- a/arch/x86/entry/entry_64_compat.S
> +++ b/arch/x86/entry/entry_64_compat.S
> @@ -213,12 +213,13 @@ sysexit_from_sys_call:
>         jnz     ia32_ret_from_sys_call
>         TRACE_IRQS_ON
>         ENABLE_INTERRUPTS(CLBR_NONE)
> +       xor     %edi, %edi
>         movl    %eax, %esi              /* second arg, syscall return value */
>         cmpl    $-MAX_ERRNO, %eax       /* is it an error ? */
>         jbe     1f

We go here if !be, which is allegedly the error case, which confuses
me, because setbe will set al (and hence edi) if be, which is also
claimed to be the error case.  Ignoring the comments for now...

>         movslq  %eax, %rsi              /* if error sign extend to 64 bits */
> -1:     setbe   %al                     /* 1 if error, 0 if not */
> -       movzbl  %al, %edi               /* zero-extend that into %edi */

Old code: edi == 1 if be and edi == 0 if !be.

> +       inc     %edi

New code: edi == 1 if !be and edi == 1 if be.

So I think that both the comment and the new code are wrong.

Am I just confused?

--Andy

> +1:     /* edi: 1 if error, 0 if not */
>         call    __audit_syscall_exit
>         movq    RAX(%rsp), %rax         /* reload syscall return value */
>         movl    $(_TIF_ALLWORK_MASK & ~_TIF_SYSCALL_AUDIT), %edi
> --
> 1.8.1.4
>



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/