From: Jiri Olsa <olsajiri@gmail.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Hillf Danton <hdanton@sina.com>,
Andy Lutomirski <luto@amacapital.net>,
Peter Anvin <hpa@zytor.com>, Adrian Bunk <bunk@kernel.org>,
syzbot <syzbot+83e7f982ca045ab4405c@syzkaller.appspotmail.com>,
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
andrii@kernel.org, bpf@vger.kernel.org,
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] x86/mm: Remove broken vsyscall emulation code from the page fault code
Date: Mon, 29 Apr 2024 15:51:47 +0200 [thread overview]
Message-ID: <Zi-l8xKhMbdJ-NBo@krava> (raw)
In-Reply-To: <Zi9Ts1HcqiKzy9GX@gmail.com>
On Mon, Apr 29, 2024 at 10:00:51AM +0200, Ingo Molnar wrote:
SNIP
> The attached patch looks like the ObviouslyCorrect(tm) thing to do.
>
> NOTE! This broken code goes back to this commit in 2011:
>
> 4fc3490114bb ("x86-64: Set siginfo and context on vsyscall emulation faults")
>
> ... and back then the reason was to get all the siginfo details right.
> Honestly, I do not for a moment believe that it's worth getting the siginfo
> details right here, but part of the commit says:
>
> This fixes issues with UML when vsyscall=emulate.
>
> ... and so my patch to remove this garbage will probably break UML in this
> situation.
>
> I do not believe that anybody should be running with vsyscall=emulate in
> 2024 in the first place, much less if you are doing things like UML. But
> let's see if somebody screams.
>
> Not-Yet-Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> Link: https://lore.kernel.org/r/CAHk-=wh9D6f7HUkDgZHKmDCHUQmp+Co89GP+b8+z+G56BKeyNg@mail.gmail.com
fwiw I can no longer trigger the invalid wait context bug
with this change
Tested-by: Jiri Olsa <jolsa@kernel.org>
jirka
> ---
> arch/x86/entry/vsyscall/vsyscall_64.c | 25 ++-----------------------
> arch/x86/include/asm/processor.h | 1 -
> arch/x86/mm/fault.c | 33 +--------------------------------
> 3 files changed, 3 insertions(+), 56 deletions(-)
>
> diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
> index a3c0df11d0e6..3b0f61b2ea6d 100644
> --- a/arch/x86/entry/vsyscall/vsyscall_64.c
> +++ b/arch/x86/entry/vsyscall/vsyscall_64.c
> @@ -98,11 +98,6 @@ static int addr_to_vsyscall_nr(unsigned long addr)
>
> static bool write_ok_or_segv(unsigned long ptr, size_t size)
> {
> - /*
> - * XXX: if access_ok, get_user, and put_user handled
> - * sig_on_uaccess_err, this could go away.
> - */
> -
> if (!access_ok((void __user *)ptr, size)) {
> struct thread_struct *thread = ¤t->thread;
>
> @@ -123,7 +118,6 @@ bool emulate_vsyscall(unsigned long error_code,
> struct task_struct *tsk;
> unsigned long caller;
> int vsyscall_nr, syscall_nr, tmp;
> - int prev_sig_on_uaccess_err;
> long ret;
> unsigned long orig_dx;
>
> @@ -234,12 +228,8 @@ bool emulate_vsyscall(unsigned long error_code,
> goto do_ret; /* skip requested */
>
> /*
> - * With a real vsyscall, page faults cause SIGSEGV. We want to
> - * preserve that behavior to make writing exploits harder.
> + * With a real vsyscall, page faults cause SIGSEGV.
> */
> - prev_sig_on_uaccess_err = current->thread.sig_on_uaccess_err;
> - current->thread.sig_on_uaccess_err = 1;
> -
> ret = -EFAULT;
> switch (vsyscall_nr) {
> case 0:
> @@ -262,23 +252,12 @@ bool emulate_vsyscall(unsigned long error_code,
> break;
> }
>
> - current->thread.sig_on_uaccess_err = prev_sig_on_uaccess_err;
> -
> check_fault:
> if (ret == -EFAULT) {
> /* Bad news -- userspace fed a bad pointer to a vsyscall. */
> warn_bad_vsyscall(KERN_INFO, regs,
> "vsyscall fault (exploit attempt?)");
> -
> - /*
> - * If we failed to generate a signal for any reason,
> - * generate one here. (This should be impossible.)
> - */
> - if (WARN_ON_ONCE(!sigismember(&tsk->pending.signal, SIGBUS) &&
> - !sigismember(&tsk->pending.signal, SIGSEGV)))
> - goto sigsegv;
> -
> - return true; /* Don't emulate the ret. */
> + goto sigsegv;
> }
>
> regs->ax = ret;
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 811548f131f4..78e51b0d6433 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -472,7 +472,6 @@ struct thread_struct {
> unsigned long iopl_emul;
>
> unsigned int iopl_warn:1;
> - unsigned int sig_on_uaccess_err:1;
>
> /*
> * Protection Keys Register for Userspace. Loaded immediately on
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 6b2ca8ba75b8..f26ecabc9424 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -724,39 +724,8 @@ kernelmode_fixup_or_oops(struct pt_regs *regs, unsigned long error_code,
> WARN_ON_ONCE(user_mode(regs));
>
> /* Are we prepared to handle this kernel fault? */
> - if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
> - /*
> - * Any interrupt that takes a fault gets the fixup. This makes
> - * the below recursive fault logic only apply to a faults from
> - * task context.
> - */
> - if (in_interrupt())
> - return;
> -
> - /*
> - * Per the above we're !in_interrupt(), aka. task context.
> - *
> - * In this case we need to make sure we're not recursively
> - * faulting through the emulate_vsyscall() logic.
> - */
> - if (current->thread.sig_on_uaccess_err && signal) {
> - sanitize_error_code(address, &error_code);
> -
> - set_signal_archinfo(address, error_code);
> -
> - if (si_code == SEGV_PKUERR) {
> - force_sig_pkuerr((void __user *)address, pkey);
> - } else {
> - /* XXX: hwpoison faults will set the wrong code. */
> - force_sig_fault(signal, si_code, (void __user *)address);
> - }
> - }
> -
> - /*
> - * Barring that, we can do the fixup and be happy.
> - */
> + if (fixup_exception(regs, X86_TRAP_PF, error_code, address))
> return;
> - }
>
> /*
> * AMD erratum #91 manifests as a spurious page fault on a PREFETCH
>
next prev parent reply other threads:[~2024-04-29 13:51 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 9:05 [syzbot] [bpf?] [trace?] possible deadlock in force_sig_info_to_task syzbot
2024-04-25 17:54 ` Jiri Olsa
2024-04-27 20:00 ` syzbot
2024-04-27 23:13 ` Hillf Danton
2024-04-28 20:01 ` Linus Torvalds
2024-04-28 20:22 ` Linus Torvalds
2024-04-28 23:23 ` Hillf Danton
2024-04-29 0:50 ` Linus Torvalds
2024-04-29 1:00 ` Tetsuo Handa
2024-04-29 1:33 ` Linus Torvalds
2024-04-29 8:00 ` [PATCH] x86/mm: Remove broken vsyscall emulation code from the page fault code Ingo Molnar
2024-04-29 13:51 ` Jiri Olsa [this message]
2024-04-29 23:30 ` Andy Lutomirski
2024-04-29 15:51 ` Linus Torvalds
2024-04-29 18:47 ` Linus Torvalds
2024-04-29 19:07 ` Linus Torvalds
2024-04-29 23:29 ` Andy Lutomirski
2024-04-30 0:05 ` Linus Torvalds
2024-04-30 6:10 ` Ingo Molnar
2024-05-01 7:43 ` Ingo Molnar
2024-04-30 14:53 ` kernel test robot
2024-04-29 10:39 ` [syzbot] [bpf?] [trace?] possible deadlock in force_sig_info_to_task Hillf Danton
2024-04-29 11:35 ` syzbot
2024-04-30 6:16 ` [tip: x86/urgent] x86/mm: Remove broken vsyscall emulation code from the page fault code tip-bot2 for Linus Torvalds
2024-05-01 7:50 ` tip-bot2 for Linus Torvalds
2024-04-29 14:17 ` [syzbot] [bpf?] [trace?] possible deadlock in force_sig_info_to_task Tetsuo Handa
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=Zi-l8xKhMbdJ-NBo@krava \
--to=olsajiri@gmail.com \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=bunk@kernel.org \
--cc=hdanton@sina.com \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=mingo@kernel.org \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=syzbot+83e7f982ca045ab4405c@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
--cc=torvalds@linux-foundation.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).