linux-kernel - Re: [PATCH 14/15] x86/fsgsbase/64: Support legacy behavior when FS/GS updated by ptracer

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: <C78DE381-C618-4370-9B06-F6E25C850661@intel.com>
Date:   Tue, 20 Mar 2018 16:33:35 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Andy Lutomirski <luto@...nel.org>
CC:     X86 ML <x86@...nel.org>, Andi Kleen <ak@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        "Metzger, Markus T" <markus.t.metzger@...el.com>,
        "Luck, Tony" <tony.luck@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 14/15] x86/fsgsbase/64: Support legacy behavior when
 FS/GS updated by ptracer

On 3/20/18, 08:05, "Andy Lutomirski" <luto@...nel.org> wrote:
> I've also suggested something like this myself, but this approach is
> far more complicated than the older approach.  Was there something
> that the old approach would break?  If so, what?
Sorry, I don't know your suggestion. Can you elaborate your suggestion?
  
>> +               /*
>> +                * %fs setting goes to reload its base, when tracee
>> +                * resumes without FSGSBASE (legacy). Here with FSGSBASE
>> +                * FS base is (manually) fetched from GDT/LDT when needed.
>> +                */
>> +               else if (static_cpu_has(X86_FEATURE_FSGSBASE) &&
>> +                        (value != 0) && (task->thread.fsindex != value))
>> +                       task->thread.fsbase = task_seg_base(task, value);

> The comment above should explain why you're checking this particular
> condition.  I find the fsindex != value check to be *very* surprising.
>  On a real CPU, writing some nonzero value to %fs does the same thing
>  regardless of what the old value of %fs was.

With FSGSBASE, when both index and base are not changed, base will
be (always) fetched from GDT/LDT. This is not thought as legacy behavior 
we need to support, AFAIK.

> This is_fully_covered thing is IMO overcomplicated.  Why not just make
> a separate helper set_fsgs_index_and_base() and have putregs() call it
> when both are set at once?

Using helper function here is exactly what I did at first. I thought this
tag is simple enough and straightforward at the end. But I'm open to
factor it out.