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 34F84C47427 for ; Tue, 29 Sep 2020 19:57:50 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 7D7102083B for ; Tue, 29 Sep 2020 19:57:49 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="vGZY5Xah" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7D7102083B 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 D40C48E0001; Tue, 29 Sep 2020 15:57:48 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D18856B005D; Tue, 29 Sep 2020 15:57:48 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C2FF98E0001; Tue, 29 Sep 2020 15:57:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0105.hostedemail.com [216.40.44.105]) by kanga.kvack.org (Postfix) with ESMTP id AE6376B005C for ; Tue, 29 Sep 2020 15:57:48 -0400 (EDT) Received: from smtpin19.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 670BC4DC7 for ; Tue, 29 Sep 2020 19:57:48 +0000 (UTC) X-FDA: 77317159416.19.water63_25078c22718c Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin19.hostedemail.com (Postfix) with ESMTP id 2E2241ACEAD for ; Tue, 29 Sep 2020 19:57:48 +0000 (UTC) X-HE-Tag: water63_25078c22718c X-Filterd-Recvd-Size: 8691 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf17.hostedemail.com (Postfix) with ESMTP for ; Tue, 29 Sep 2020 19:57:47 +0000 (UTC) Received: from mail-wr1-f49.google.com (mail-wr1-f49.google.com [209.85.221.49]) (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 0815E21D7D for ; Tue, 29 Sep 2020 19:57:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601409466; bh=Qwu5PoL63lD9i/Fv1xhrZCI4FoJQXvwA0rtPHDOgnU8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=vGZY5XahMr5R+WKF6EfFh1JVk4HZVMZzR7QS4bPnpAPvPTX+f+smn0YBpAU5zlTPX 3J/GZNFCqc4hBT0ImF3z/Ea3WX29Vxa6HXJL7k6fj55kMIbrw2h1jBjMjbY+z9JIH/ Rl1zj4dQALVQuxsfJSKNbi23Qp6NHVHNk+eXKAfI= Received: by mail-wr1-f49.google.com with SMTP id z1so6819600wrt.3 for ; Tue, 29 Sep 2020 12:57:45 -0700 (PDT) X-Gm-Message-State: AOAM532iwS/K0zQLuc+UXkMOHT6CKPw/oIje361qkyK8Ed0Bui7lzuNA BVpWAiKgWpXpSqbBSqcBmlG8v87b0CWFBrS3dzvzpQ== X-Google-Smtp-Source: ABdhPJx3lB2zlueCAMn75sMTUFY2AhvOfwqVqFgs8RvW9w4m++tuwShPo2eZ5RRLRFyI+R3rSLBO0y7Mo3n373Ou2ZQ= X-Received: by 2002:adf:a3c3:: with SMTP id m3mr6065511wrb.70.1601409464264; Tue, 29 Sep 2020 12:57:44 -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 12:57:32 -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: "Yu, Yu-cheng" Cc: Andy Lutomirski , 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 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. --Andy