From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 64ADFC4727F for ; Tue, 29 Sep 2020 20:01:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 044CA2067C for ; Tue, 29 Sep 2020 20:01:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601409670; bh=VdAwB85Jl6jjnWW1rC86bh2TRgL/h2uAnUFh0PPFMr4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-ID:From; b=Ud4HwaWJ9L/6PGPwVXJ0fQUgyLYVar6RD9mVfTljl16G1GhO3ykGFIrG+uHpDGQKQ aJ8ropg6GtrnHdAXeXYexxRMtzFmOPfNDSqshHAdCiGQ0Zs1B7lT7UiQczS6NB5aDI su/tXA6IbWYgS3iF/yUWPQIZZaijf2bmPVZJKgqo= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728963AbgI2UBJ (ORCPT ); Tue, 29 Sep 2020 16:01:09 -0400 Received: from mail.kernel.org ([198.145.29.99]:52028 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728825AbgI2UBH (ORCPT ); Tue, 29 Sep 2020 16:01:07 -0400 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 869472184D for ; Tue, 29 Sep 2020 20:01:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601409666; bh=VdAwB85Jl6jjnWW1rC86bh2TRgL/h2uAnUFh0PPFMr4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=cfdXigA6DKc6rUVWXz2HkTTAVHFK2u4oD2afzFAEDTqSJkRE3Q5dtTQuX7LK+FTqN 9+fMmUb+HYDN5S1ssEhWegXnE3VGr0Vasy6cNRqdVHTNtZCg8TNELu7qp/DUB38K9F l9uVkrvMA5plbPjskmdqZQovD6jblgJnJZE16zAw= Received: by mail-wm1-f41.google.com with SMTP id e2so6138807wme.1 for ; Tue, 29 Sep 2020 13:01:06 -0700 (PDT) X-Gm-Message-State: AOAM533OaROzOcSTJZeDHgzizyH4rpFX6wdEVqWYi0CHnYbuemSp9e1h 1hBIg9D5nSf7HH3ziehzzzgq67hYhMEiIEXsRR/jYQ== X-Google-Smtp-Source: ABdhPJxSIxQqxZfr1Lddd+J2JuqnCdsUEPeddo75CLB979xi4dT2T9iOuaSovNDW2MTrs8626F2sx3ezpS7no2PeGnM= X-Received: by 2002:a05:600c:4104:: with SMTP id j4mr6098469wmi.36.1601409664710; Tue, 29 Sep 2020 13:01:04 -0700 (PDT) MIME-Version: 1.0 References: <99B32E59-CFF2-4756-89BD-AEA0021F355F@amacapital.net> In-Reply-To: From: Andy Lutomirski Date: Tue, 29 Sep 2020 13:00:53 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v13 8/8] x86/vsyscall/64: Fixup Shadow Stack and Indirect Branch Tracking for vsyscall emulation To: Andy Lutomirski Cc: "Yu, Yu-cheng" , X86 ML , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , LKML , "open list:DOCUMENTATION" , Linux-MM , linux-arch , Linux API , Arnd Bergmann , Balbir Singh , Borislav Petkov , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H.J. Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , "Ravi V. Shankar" , Vedvyas Shanbhogue , Dave Martin , Weijiang Yang , Pengfei Xu Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 29, 2020 at 12:57 PM Andy Lutomirski wrote: > > On Tue, Sep 29, 2020 at 11:37 AM Yu, Yu-cheng wrote: > > > > On 9/28/2020 10:37 AM, Andy Lutomirski wrote: > > > On Mon, Sep 28, 2020 at 9:59 AM Yu-cheng Yu wrote: > > >> > > >> On Fri, 2020-09-25 at 09:51 -0700, Andy Lutomirski wrote: > > >>>> On Sep 25, 2020, at 9:48 AM, Yu, Yu-cheng wrote: > > >> + > > >> + cet = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); > > >> + if (!cet) { > > >> + /* > > >> + * This is an unlikely case where the task is > > >> + * CET-enabled, but CET xstate is in INIT. > > >> + */ > > >> + WARN_ONCE(1, "CET is enabled, but no xstates"); > > > > > > "unlikely" doesn't really cover this. > > > > > >> + fpregs_unlock(); > > >> + goto sigsegv; > > >> + } > > >> + > > >> + if (cet->user_ssp && ((cet->user_ssp + 8) < TASK_SIZE_MAX)) > > >> + cet->user_ssp += 8; > > > > > > This looks buggy. The condition should be "if SHSTK is on, then add 8 > > > to user_ssp". If the result is noncanonical, then some appropriate > > > exception should be generated, probably by the FPU restore code -- see > > > below. You should be checking the SHSTK_EN bit, not SSP. > > > > Updated. Is this OK? I will resend the whole series later. > > > > Thanks, > > Yu-cheng > > > > ====== > > > > From 09803e66dca38d7784e32687d0693550948199ed Mon Sep 17 00:00:00 2001 > > From: Yu-cheng Yu > > Date: Thu, 29 Nov 2018 14:15:38 -0800 > > Subject: [PATCH v13 8/8] x86/vsyscall/64: Fixup Shadow Stack and > > Indirect Branch > > Tracking for vsyscall emulation > > > > Vsyscall entry points are effectively branch targets. Mark them with > > ENDBR64 opcodes. When emulating the RET instruction, unwind shadow stack > > and reset IBT state machine. > > > > Signed-off-by: Yu-cheng Yu > > --- > > v13: > > - Check shadow stack address is canonical. > > - Change from writing to MSRs to writing to CET xstate. > > > > arch/x86/entry/vsyscall/vsyscall_64.c | 34 +++++++++++++++++++++++ > > arch/x86/entry/vsyscall/vsyscall_emu_64.S | 9 ++++++ > > arch/x86/entry/vsyscall/vsyscall_trace.h | 1 + > > 3 files changed, 44 insertions(+) > > > > diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c > > b/arch/x86/entry/vsyscall/vsyscall_64.c > > index 44c33103a955..30b166091d46 100644 > > --- a/arch/x86/entry/vsyscall/vsyscall_64.c > > +++ b/arch/x86/entry/vsyscall/vsyscall_64.c > > @@ -38,6 +38,9 @@ > > #include > > #include > > #include > > +#include > > +#include > > +#include > > > > #define CREATE_TRACE_POINTS > > #include "vsyscall_trace.h" > > @@ -286,6 +289,44 @@ bool emulate_vsyscall(unsigned long error_code, > > /* Emulate a ret instruction. */ > > regs->ip = caller; > > regs->sp += 8; > > + > > +#ifdef CONFIG_X86_CET > > + if (tsk->thread.cet.shstk_size || tsk->thread.cet.ibt_enabled) { > > + struct cet_user_state *cet; > > + struct fpu *fpu; > > + > > + fpu = &tsk->thread.fpu; > > + fpregs_lock(); > > + > > + if (!test_thread_flag(TIF_NEED_FPU_LOAD)) { > > + copy_fpregs_to_fpstate(fpu); > > + set_thread_flag(TIF_NEED_FPU_LOAD); > > + } > > + > > + cet = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); > > + if (!cet) { > > + /* > > + * This should not happen. The task is > > + * CET-enabled, but CET xstate is in INIT. > > + */ > > Can the comment explain better, please? I would say something like: > > If the kernel thinks this task has CET enabled (because > tsk->thread.cet has one of the features enabled), then the > corresponding bits must also be set in the CET XSAVES region. If the > CET XSAVES region is in the INIT state, then the kernel's concept of > the task's CET state is corrupt. > > > + WARN_ONCE(1, "CET is enabled, but no xstates"); > > + fpregs_unlock(); > > + goto sigsegv; > > + } > > + > > + if (cet->user_cet & CET_SHSTK_EN) { > > + if (cet->user_ssp && (cet->user_ssp + 8 < TASK_SIZE_MAX)) > > + cet->user_ssp += 8; > > + } > > This makes so sense to me. Also, the vsyscall emulation code is > intended to be as rigid as possible to minimize the chance that it > gets used as an exploit gadget. So we should not silently corrupt > anything. Moreover, this code seems quite dangerous -- you've created > a gadget that does RET without actually verifying the SHSTK token. If > SHSTK and some form of strong indirect branch/call CFI is in use, then > the existance of a CFI-bypassing return primitive at a fixed address > seems quite problematic. > > So I think you need to write a function that reasonably accurately > emulates a usermode RET. > For what it's worth, I think there is an alternative. If you all (userspace people, etc) can come up with a credible way for a user program to statically declare that it doesn't need vsyscalls, then we could make SHSTK depend on *that*, and we could avoid this mess. This breaks orthogonality, but it's probably a decent outcome. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90953C47420 for ; Tue, 29 Sep 2020 20:01:10 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id F0C7F20679 for ; Tue, 29 Sep 2020 20:01:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="cfdXigA6" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F0C7F20679 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 596526B005D; Tue, 29 Sep 2020 16:01:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 546DA6B0062; Tue, 29 Sep 2020 16:01:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 45D0C6B0068; Tue, 29 Sep 2020 16:01:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0133.hostedemail.com [216.40.44.133]) by kanga.kvack.org (Postfix) with ESMTP id 2E6F16B005D for ; Tue, 29 Sep 2020 16:01:09 -0400 (EDT) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id CF4324DD0 for ; Tue, 29 Sep 2020 20:01:08 +0000 (UTC) X-FDA: 77317167816.02.pump69_56175002718c Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin02.hostedemail.com (Postfix) with ESMTP id 9A7AE10188D06 for ; Tue, 29 Sep 2020 20:01:08 +0000 (UTC) X-HE-Tag: pump69_56175002718c X-Filterd-Recvd-Size: 9449 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf13.hostedemail.com (Postfix) with ESMTP for ; Tue, 29 Sep 2020 20:01:07 +0000 (UTC) Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8E32721D41 for ; Tue, 29 Sep 2020 20:01:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601409666; bh=VdAwB85Jl6jjnWW1rC86bh2TRgL/h2uAnUFh0PPFMr4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=cfdXigA6DKc6rUVWXz2HkTTAVHFK2u4oD2afzFAEDTqSJkRE3Q5dtTQuX7LK+FTqN 9+fMmUb+HYDN5S1ssEhWegXnE3VGr0Vasy6cNRqdVHTNtZCg8TNELu7qp/DUB38K9F l9uVkrvMA5plbPjskmdqZQovD6jblgJnJZE16zAw= Received: by mail-wm1-f41.google.com with SMTP id e17so5879471wme.0 for ; Tue, 29 Sep 2020 13:01:06 -0700 (PDT) X-Gm-Message-State: AOAM533w+4IohovDR670or3IgR2mF+jZNd6uc31sr9LrRTxbxw2Pl5Yu u+klGpcS7PD+y8kyTwKILcLqwC2SfTV9gs57hUQL1Q== X-Google-Smtp-Source: ABdhPJxSIxQqxZfr1Lddd+J2JuqnCdsUEPeddo75CLB979xi4dT2T9iOuaSovNDW2MTrs8626F2sx3ezpS7no2PeGnM= X-Received: by 2002:a05:600c:4104:: with SMTP id j4mr6098469wmi.36.1601409664710; Tue, 29 Sep 2020 13:01:04 -0700 (PDT) MIME-Version: 1.0 References: <99B32E59-CFF2-4756-89BD-AEA0021F355F@amacapital.net> In-Reply-To: From: Andy Lutomirski Date: Tue, 29 Sep 2020 13:00:53 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v13 8/8] x86/vsyscall/64: Fixup Shadow Stack and Indirect Branch Tracking for vsyscall emulation To: Andy Lutomirski Cc: "Yu, Yu-cheng" , X86 ML , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , LKML , "open list:DOCUMENTATION" , Linux-MM , linux-arch , Linux API , Arnd Bergmann , Balbir Singh , Borislav Petkov , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H.J. Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , "Ravi V. Shankar" , Vedvyas Shanbhogue , Dave Martin , Weijiang Yang , Pengfei Xu Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Sep 29, 2020 at 12:57 PM Andy Lutomirski wrote: > > On Tue, Sep 29, 2020 at 11:37 AM Yu, Yu-cheng wrote: > > > > On 9/28/2020 10:37 AM, Andy Lutomirski wrote: > > > On Mon, Sep 28, 2020 at 9:59 AM Yu-cheng Yu wrote: > > >> > > >> On Fri, 2020-09-25 at 09:51 -0700, Andy Lutomirski wrote: > > >>>> On Sep 25, 2020, at 9:48 AM, Yu, Yu-cheng wrote: > > >> + > > >> + cet = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); > > >> + if (!cet) { > > >> + /* > > >> + * This is an unlikely case where the task is > > >> + * CET-enabled, but CET xstate is in INIT. > > >> + */ > > >> + WARN_ONCE(1, "CET is enabled, but no xstates"); > > > > > > "unlikely" doesn't really cover this. > > > > > >> + fpregs_unlock(); > > >> + goto sigsegv; > > >> + } > > >> + > > >> + if (cet->user_ssp && ((cet->user_ssp + 8) < TASK_SIZE_MAX)) > > >> + cet->user_ssp += 8; > > > > > > This looks buggy. The condition should be "if SHSTK is on, then add 8 > > > to user_ssp". If the result is noncanonical, then some appropriate > > > exception should be generated, probably by the FPU restore code -- see > > > below. You should be checking the SHSTK_EN bit, not SSP. > > > > Updated. Is this OK? I will resend the whole series later. > > > > Thanks, > > Yu-cheng > > > > ====== > > > > From 09803e66dca38d7784e32687d0693550948199ed Mon Sep 17 00:00:00 2001 > > From: Yu-cheng Yu > > Date: Thu, 29 Nov 2018 14:15:38 -0800 > > Subject: [PATCH v13 8/8] x86/vsyscall/64: Fixup Shadow Stack and > > Indirect Branch > > Tracking for vsyscall emulation > > > > Vsyscall entry points are effectively branch targets. Mark them with > > ENDBR64 opcodes. When emulating the RET instruction, unwind shadow stack > > and reset IBT state machine. > > > > Signed-off-by: Yu-cheng Yu > > --- > > v13: > > - Check shadow stack address is canonical. > > - Change from writing to MSRs to writing to CET xstate. > > > > arch/x86/entry/vsyscall/vsyscall_64.c | 34 +++++++++++++++++++++++ > > arch/x86/entry/vsyscall/vsyscall_emu_64.S | 9 ++++++ > > arch/x86/entry/vsyscall/vsyscall_trace.h | 1 + > > 3 files changed, 44 insertions(+) > > > > diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c > > b/arch/x86/entry/vsyscall/vsyscall_64.c > > index 44c33103a955..30b166091d46 100644 > > --- a/arch/x86/entry/vsyscall/vsyscall_64.c > > +++ b/arch/x86/entry/vsyscall/vsyscall_64.c > > @@ -38,6 +38,9 @@ > > #include > > #include > > #include > > +#include > > +#include > > +#include > > > > #define CREATE_TRACE_POINTS > > #include "vsyscall_trace.h" > > @@ -286,6 +289,44 @@ bool emulate_vsyscall(unsigned long error_code, > > /* Emulate a ret instruction. */ > > regs->ip = caller; > > regs->sp += 8; > > + > > +#ifdef CONFIG_X86_CET > > + if (tsk->thread.cet.shstk_size || tsk->thread.cet.ibt_enabled) { > > + struct cet_user_state *cet; > > + struct fpu *fpu; > > + > > + fpu = &tsk->thread.fpu; > > + fpregs_lock(); > > + > > + if (!test_thread_flag(TIF_NEED_FPU_LOAD)) { > > + copy_fpregs_to_fpstate(fpu); > > + set_thread_flag(TIF_NEED_FPU_LOAD); > > + } > > + > > + cet = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); > > + if (!cet) { > > + /* > > + * This should not happen. The task is > > + * CET-enabled, but CET xstate is in INIT. > > + */ > > Can the comment explain better, please? I would say something like: > > If the kernel thinks this task has CET enabled (because > tsk->thread.cet has one of the features enabled), then the > corresponding bits must also be set in the CET XSAVES region. If the > CET XSAVES region is in the INIT state, then the kernel's concept of > the task's CET state is corrupt. > > > + WARN_ONCE(1, "CET is enabled, but no xstates"); > > + fpregs_unlock(); > > + goto sigsegv; > > + } > > + > > + if (cet->user_cet & CET_SHSTK_EN) { > > + if (cet->user_ssp && (cet->user_ssp + 8 < TASK_SIZE_MAX)) > > + cet->user_ssp += 8; > > + } > > This makes so sense to me. Also, the vsyscall emulation code is > intended to be as rigid as possible to minimize the chance that it > gets used as an exploit gadget. So we should not silently corrupt > anything. Moreover, this code seems quite dangerous -- you've created > a gadget that does RET without actually verifying the SHSTK token. If > SHSTK and some form of strong indirect branch/call CFI is in use, then > the existance of a CFI-bypassing return primitive at a fixed address > seems quite problematic. > > So I think you need to write a function that reasonably accurately > emulates a usermode RET. > For what it's worth, I think there is an alternative. If you all (userspace people, etc) can come up with a credible way for a user program to statically declare that it doesn't need vsyscalls, then we could make SHSTK depend on *that*, and we could avoid this mess. This breaks orthogonality, but it's probably a decent outcome.