Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH RFC v2 4/6] x86: Disable PTI on compatibility mode
From:   Nadav Amit <nadav.amit@gmail.com>
In-Reply-To: <CALCETrUY4uZeG2fz=OQWGuyCOcBBNbep6TaJz82CZSP89=g3-A@mail.gmail.com>
Date:   Thu, 15 Feb 2018 12:58:38 -0800
Cc:     Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Willy Tarreau <w@1wt.eu>, X86 ML <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <9EB804CA-0EC9-4CBB-965A-F3C8520201E7@gmail.com>
References: <20180215163602.61162-1-namit@vmware.com>
 <20180215163602.61162-5-namit@vmware.com>
 <CALCETrUY4uZeG2fz=OQWGuyCOcBBNbep6TaJz82CZSP89=g3-A@mail.gmail.com>
To:     Andy Lutomirski <luto@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Andy Lutomirski <luto@kernel.org> wrote:

> On Thu, Feb 15, 2018 at 4:36 PM, Nadav Amit <namit@vmware.com> wrote:
>> Based on the understanding that there should be no way for userspace =
to
>> address the kernel-space from compatibility mode, disable it while
>> running in compatibility mode as long as the 64-bit code segment of =
the
>> user is not used.
>>=20
>> Reenabling PTI is performed by restoring NX-bits to the userspace
>> mappings, flushing the TLBs, and notifying all the CPUs that use the
>> affected mm to disable PTI. Each core responds by removing the =
present
>> bit for the 64-bit code-segment, and marking that PTI is disabled on
>> that core.
>=20
> I dislike this patch because it's conflating two things.  The patch
> claims to merely disable PTI for compat tasks, whatever those are.
> But it's also introducing a much stronger concept of what a compat
> task is.  The kernel currently mostly doesn't care whether a task is
> "compat" or not, and I think that most remaining code paths that do
> care are buggy and should be removed.
>=20
> I think the right way to approach this is to add a new arch_prctl()
> that changes allowable bitness, like this:
>=20
> arch_prctl(ARCH_SET_ALLOWED_GDT_CS, X86_ALLOW_CS32 | X86_ALLOW_CS64);
>=20
> this would set the current task to work the normal way, where 32-bit
> and 64-bit CS are available.  You could set just X86_ALLOW_CS32 to
> deny 64-bit mode and just X86_ALLOW_CS64 to deny 32-bit mode.  This
> would make nice attack surface reduction tools for the more paranoid
> sandbox users to use.  Doing arch_prctl(ARCH_SET_ALLOWED_GDT_CS, 0)
> would return -EINVAL.
>=20
> A separate patch would turn PTI off if you set X86_ALLOW_CS32.
>=20
> This has the downside that old code doesn't get the benefit without
> some code change, but that's not the end of the world.
>=20
>> +static void pti_cpu_update_func(void *info)
>> +{
>> +       struct mm_struct *mm =3D (struct mm_struct *)info;
>> +
>> +       if (mm !=3D this_cpu_read(cpu_tlbstate.loaded_mm))
>> +               return;
>> +
>> +       /*
>> +        * Keep CS64 and CPU settings in sync despite potential =
concurrent
>> +        * updates.
>> +        */
>> +       set_cpu_pti_disable(READ_ONCE(mm->context.pti_disable));
>> +}
>=20
> I don't like this at all.  IMO a sane implementation should never
> change PTI status on a remote CPU.  Just track it per task.

=46rom your comments in the past, I understood that this requirement
(enabling/disabling PTI per mm) came from Linus. I can do it per task, =
but
that means that the NX-bit will always be cleared on the kernel =
page-table
for user mappings.

>> +void __pti_reenable(void)
>> +{
>> +       struct mm_struct *mm =3D current->mm;
>> +       int cpu;
>> +
>> +       if (!mm_pti_disable(mm))
>> +               return;
>> +
>> +       /*
>> +        * Prevent spurious page-fault storm while we set the NX-bit =
and have
>> +        * yet not updated the per-CPU pti_disable flag.
>> +        */
>> +       down_write(&mm->mmap_sem);
>> +
>> +       if (!mm_pti_disable(mm))
>> +               goto out;
>> +
>> +       /*
>> +        * First, mark the PTI is enabled. Although we do anything =
yet, we are
>> +        * safe as long as we do not reenable CS64. Since we did not =
update the
>> +        * page tables yet, this may lead to spurious page-faults, =
but we need
>> +        * the pti_disable in mm to be set for __pti_set_user_pgd() =
to do the
>> +        * right thing.  Holding mmap_sem would ensure matter we hold =
the
>> +        * mmap_sem to prevent them from swamping the system.
>> +        */
>> +       mm->context.pti_disable =3D PTI_DISABLE_OFF;
>> +
>> +       /* Second, restore the NX bits. */
>> +       pti_update_user_pgds(mm, true);
>=20
> You're holding mmap_sem, but there are code paths that touch page
> tables that don't hold mmap_sem, such as the stack extension code.

Do they change user mappings? These are the only ones =
pti_update_user_pgds()
changes.

>> +
>> +bool pti_handle_segment_not_present(long error_code)
>> +{
>> +       if (!static_cpu_has(X86_FEATURE_PTI))
>> +               return false;
>> +
>> +       if ((unsigned short)error_code !=3D GDT_ENTRY_DEFAULT_USER_CS =
<< 3)
>> +               return false;
>> +
>> +       pti_reenable();
>> +       return true;
>> +}
>=20
> Please don't.  You're trying to emulate the old behavior here, but
> you're emulating it wrong.  In particular, you won't trap on LAR.

Yes, I thought I=E2=80=99ll manage to address LAR, but failed. I thought =
you said
this is not a =E2=80=9Cshow-stopper=E2=80=9D. I=E2=80=99ll adapt your =
approach of using prctl, although
it really limits the benefit of this mechanism.