From: Andy Lutomirski <luto@kernel.org>
To: Sasha Levin <sashal@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Borislav Petkov <bp@alien8.de>,
Andrew Lutomirski <luto@kernel.org>,
"H. Peter Anvin" <hpa@zytor.com>,
Dave Hansen <dave.hansen@intel.com>,
Tony Luck <tony.luck@intel.com>, Andi Kleen <ak@linux.intel.com>,
"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
"Bae, Chang Seok" <chang.seok.bae@intel.com>
Subject: Re: [PATCH v10 01/18] x86/ptrace: Prevent ptrace from clearing the FS/GS selector
Date: Sat, 25 Apr 2020 15:46:56 -0700 [thread overview]
Message-ID: <CALCETrUdmN3az78Aqfcta+waUOfW1=DXxw-M-t4m1zGdBnHGmg@mail.gmail.com> (raw)
In-Reply-To: <20200423232207.5797-2-sashal@kernel.org>
On Thu, Apr 23, 2020 at 4:22 PM Sasha Levin <sashal@kernel.org> wrote:
>
> From: "Chang S. Bae" <chang.seok.bae@intel.com>
>
> When a ptracer writes a ptracee's FS/GS base with a different value, the
> selector is also cleared. This behavior is not correct as the selector
> should be preserved.
>
> Update only the base value and leave the selector intact. To simplify the
> code further remove the conditional checking for the same value as this
> code is not performance-critical.
>
> The only recognizable downside of this change is when the selector is
> already nonzero on write. The base will be reloaded according to the
> selector. But the case is highly unexpected in real usages.
After spending a while reading this patch, I think it's probably okay,
but this ptrace stuff is utter garbage. The changelog should explain
why common cases work with the current code, what you think the point
(if any) of the condition you're removing is, and why it's okay to
make this change.
Certainly the current changelog is wrong. You say "The base will be
reloaded according to the selector". The code you're changing calls
x86_fs/gsbase_write_task(), which is, effectively:
task->thread.fsbase = fsbase;
This doesn't reload anything.
Maybe what you're trying to say is "with this patch applied, as is or
with FSGSBASE disabled, if the tracee has FS != 0 and a tracer
modifies only fs_base, then the change won't stick."
--Andy
next prev parent reply other threads:[~2020-04-25 22:48 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-23 23:21 [PATCH v10 00/18] Enable FSGSBASE instructions Sasha Levin
2020-04-23 23:21 ` [PATCH v10 01/18] x86/ptrace: Prevent ptrace from clearing the FS/GS selector Sasha Levin
2020-04-25 22:46 ` Andy Lutomirski [this message]
2020-04-23 23:21 ` [PATCH v10 02/18] selftests/x86/fsgsbase: Test GS selector on ptracer-induced GS base write Sasha Levin
2020-04-23 23:21 ` [PATCH v10 03/18] x86/cpu: Add 'unsafe_fsgsbase' to enable CR4.FSGSBASE Sasha Levin
2020-04-23 23:21 ` [PATCH v10 04/18] x86/entry/64: Clean up paranoid exit Sasha Levin
2020-04-23 23:21 ` [PATCH v10 05/18] x86/entry/64: Switch CR3 before SWAPGS in paranoid entry Sasha Levin
2020-04-23 23:21 ` [PATCH v10 06/18] x86/entry/64: Introduce the FIND_PERCPU_BASE macro Sasha Levin
2020-04-23 23:21 ` [PATCH v10 07/18] x86/entry/64: Handle FSGSBASE enabled paranoid entry/exit Sasha Levin
2020-04-23 23:21 ` [PATCH v10 08/18] x86/entry/64: Document GSBASE handling in the paranoid path Sasha Levin
2020-04-23 23:21 ` [PATCH v10 09/18] x86/fsgsbase/64: Add intrinsics for FSGSBASE instructions Sasha Levin
2020-04-23 23:21 ` [PATCH v10 10/18] x86/fsgsbase/64: Enable FSGSBASE instructions in helper functions Sasha Levin
2020-04-23 23:22 ` [PATCH v10 11/18] x86/fsgsbase/64: Use FSGSBASE in switch_to() if available Sasha Levin
2020-04-23 23:22 ` [PATCH v10 12/18] x86/fsgsbase/64: move save_fsgs to header file Sasha Levin
2020-04-23 23:22 ` [PATCH v10 13/18] x86/fsgsbase/64: Use FSGSBASE instructions on thread copy and ptrace Sasha Levin
2020-04-23 23:22 ` [PATCH v10 14/18] x86/speculation/swapgs: Check FSGSBASE in enabling SWAPGS mitigation Sasha Levin
2020-04-23 23:22 ` [PATCH v10 15/18] selftests/x86/fsgsbase: Test ptracer-induced GS base write with FSGSBASE Sasha Levin
2020-04-23 23:22 ` [PATCH v10 16/18] x86/fsgsbase/64: Enable FSGSBASE on 64bit by default and add a chicken bit Sasha Levin
2020-04-23 23:22 ` [PATCH v10 17/18] x86/elf: Enumerate kernel FSGSBASE capability in AT_HWCAP2 Sasha Levin
2020-04-23 23:22 ` [PATCH v10 18/18] Documentation/x86/64: Add documentation for GS/FS addressing mode Sasha Levin
2020-05-10 8:09 ` [PATCH v10 00/18] Enable FSGSBASE instructions Vegard Nossum
2020-05-10 8:29 ` Vegard Nossum
2020-05-10 10:15 ` Thomas Gleixner
2020-05-10 14:17 ` Sasha Levin
2020-05-11 0:48 ` Andi Kleen
2020-05-11 0:50 ` Andi Kleen
2020-05-11 5:03 ` Sasha Levin
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='CALCETrUdmN3az78Aqfcta+waUOfW1=DXxw-M-t4m1zGdBnHGmg@mail.gmail.com' \
--to=luto@kernel.org \
--cc=ak@linux.intel.com \
--cc=bp@alien8.de \
--cc=chang.seok.bae@intel.com \
--cc=dave.hansen@intel.com \
--cc=hpa@zytor.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ravi.v.shankar@intel.com \
--cc=sashal@kernel.org \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
/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).