From: Linus Torvalds <torvalds@linux-foundation.org>
To: Andy Lutomirski <luto@amacapital.net>
Cc: Denys Vlasenko <dvlasenk@redhat.com>,
Kees Cook <keescook@chromium.org>,
David Drysdale <drysdale@google.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Will Drewry <wad@chromium.org>, Ingo Molnar <mingo@kernel.org>,
Alok Kataria <akataria@vmware.com>,
Borislav Petkov <bp@alien8.de>,
Alexei Starovoitov <ast@plumgrid.com>,
Frederic Weisbecker <fweisbec@gmail.com>,
"H. Peter Anvin" <hpa@zytor.com>, Oleg Nesterov <oleg@redhat.com>,
Steven Rostedt <rostedt@goodmis.org>, X86 ML <x86@kernel.org>
Subject: Re: [Regression v4.2 ?] 32-bit seccomp-BPF returned errno values wrong in VM?
Date: Thu, 13 Aug 2015 15:49:54 -0700 [thread overview]
Message-ID: <CA+55aFyA9+16Jer2nKMOs6_uhRA-egvFDSR8fgNoD9AuPLE4GQ@mail.gmail.com> (raw)
In-Reply-To: <CALCETrWj6X+zCX47iHip0Qjp88p7f3p0uWvskL357XCyvPrXVA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]
On Thu, Aug 13, 2015 at 2:47 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> It seems to me that the bug is that sysexit_from_sys_call isn't
> reloading RAX from regs->ax.
Ugh. That code is confusing, and _most_ cases seem to have %rax already loaded.
There seems to be three cases:
- fallthrough from cstar_dispatch after a successful call to the system call
This has %rax as the correct return value (which also got saved to
RAX on stack)
- the 'auditsys_exit' macro 'exit' case.
This seems to have %rax reloaded inside the macro
- the out-of-range system call case for cstar_dispatch
This does *not* seem to load %rax with $ENOSYS, but keeps the bad
system call number in it.
so yeah, there seems to be a bug there, but if I read it right, that
bug seems to happen just for the out-of-range system call case, which
afaik isn't the case reported here.
I guess adding a re-load of %rax is ok, even though in the common
cases it is already loaded.
Oh, and sysexit_from_sys_call seems to have the exact same situation.
The "system call dispatch with %rax out of range" fallthrough case
doesn't set %rax to ENOSYS.
So I guess we could remove the reloading of system call return value
from auditsys_exit, and just do it unconditionally in the common path.
Which is sad, since the *really* common case already has the right
value, but whatever.
Does the attached patch make sense and work? Totally untested, just
looking at the code. But maybe it's right, because it's exactly that
ENOSYS case that the bad patch in question changed.
Btw, the old ENOSYS code also cleared ORIG_EAX. I'm not sure why, but
we used to have
ia32_badsys:
movq $0,ORIG_RAX(%rsp)
movq $-ENOSYS,%rax
jmp ia32_sysret
for that case.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1084 bytes --]
arch/x86/entry/entry_64_compat.S | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 5a1844765a7a..a7e257d9cb90 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -140,6 +140,7 @@ sysexit_from_sys_call:
*/
andl $~TS_COMPAT, ASM_THREAD_INFO(TI_status, %rsp, SIZEOF_PTREGS)
movl RIP(%rsp), %ecx /* User %eip */
+ movq RAX(%rsp), %rax
RESTORE_RSI_RDI
xorl %edx, %edx /* Do not leak kernel information */
xorq %r8, %r8
@@ -219,7 +220,6 @@ sysexit_from_sys_call:
1: setbe %al /* 1 if error, 0 if not */
movzbl %al, %edi /* zero-extend that into %edi */
call __audit_syscall_exit
- movq RAX(%rsp), %rax /* reload syscall return value */
movl $(_TIF_ALLWORK_MASK & ~_TIF_SYSCALL_AUDIT), %edi
DISABLE_INTERRUPTS(CLBR_NONE)
TRACE_IRQS_OFF
@@ -368,6 +368,7 @@ sysretl_from_sys_call:
RESTORE_RSI_RDI_RDX
movl RIP(%rsp), %ecx
movl EFLAGS(%rsp), %r11d
+ movq RAX(%rsp), %rax
xorq %r10, %r10
xorq %r9, %r9
xorq %r8, %r8
next prev parent reply other threads:[~2015-08-13 22:49 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-13 8:30 [Regression v4.2 ?] 32-bit seccomp-BPF returned errno values wrong in VM? David Drysdale
2015-08-13 15:17 ` Denys Vlasenko
2015-08-13 16:28 ` David Drysdale
2015-08-13 17:15 ` Andy Lutomirski
2015-08-13 17:39 ` David Drysdale
2015-08-13 18:47 ` Kees Cook
2015-08-13 21:35 ` Denys Vlasenko
2015-08-13 21:47 ` Andy Lutomirski
2015-08-13 22:49 ` Linus Torvalds [this message]
2015-08-13 22:54 ` Linus Torvalds
2015-08-13 22:56 ` Kees Cook
2015-08-13 22:59 ` Andy Lutomirski
2015-08-13 23:14 ` Kees Cook
2015-08-13 23:30 ` Linus Torvalds
2015-08-14 11:58 ` Denys Vlasenko
2015-08-14 14:27 ` Andy Lutomirski
2015-08-14 7:33 ` David Drysdale
2015-08-13 22:58 ` Andy Lutomirski
2015-08-13 23:25 ` Linus Torvalds
2015-08-13 22:27 ` Linus Torvalds
2015-08-14 11:20 ` Denys Vlasenko
2015-08-22 10:03 ` Ingo Molnar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+55aFyA9+16Jer2nKMOs6_uhRA-egvFDSR8fgNoD9AuPLE4GQ@mail.gmail.com \
--to=torvalds@linux-foundation.org \
--cc=akataria@vmware.com \
--cc=ast@plumgrid.com \
--cc=bp@alien8.de \
--cc=drysdale@google.com \
--cc=dvlasenk@redhat.com \
--cc=fweisbec@gmail.com \
--cc=hpa@zytor.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=mingo@kernel.org \
--cc=oleg@redhat.com \
--cc=rostedt@goodmis.org \
--cc=wad@chromium.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).