linux-kernel - Re: [PATCH v10 01/18] x86/ptrace: Prevent ptrace from clearing the FS/GS selector

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: <CALCETrUdmN3az78Aqfcta+waUOfW1=DXxw-M-t4m1zGdBnHGmg@mail.gmail.com>
Date:   Sat, 25 Apr 2020 15:46:56 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Sasha Levin <sashal@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        Andrew Lutomirski <luto@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        "Bae, Chang Seok" <chang.seok.bae@...el.com>
Subject: Re: [PATCH v10 01/18] x86/ptrace: Prevent ptrace from clearing the
 FS/GS selector

On Thu, Apr 23, 2020 at 4:22 PM Sasha Levin <sashal@...nel.org> wrote:
>
> From: "Chang S. Bae" <chang.seok.bae@...el.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