From patchwork Fri Dec 15 18:34:38 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Martin X-Patchwork-Id: 10115843 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 515A960327 for ; Fri, 15 Dec 2017 18:35:42 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 497152900D for ; Fri, 15 Dec 2017 18:35:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3CC9629039; Fri, 15 Dec 2017 18:35:42 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id C64C72900D for ; Fri, 15 Dec 2017 18:35:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:MIME-Version:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id:Message-Id:Date: Subject:To:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To: References:List-Owner; bh=+ngymCc7VuTPurgu45U7fqOaEpfqYsSyRT9eQMUzNjE=; b=epR nrA1xQge+9MwWPpc03gE1zjiTAvFwT2pDLbZuJusln3y8PcJSX+dFHBAQXaBwOl56NTgxMntxmnPr fv3hLWjTmeregrhrXYDkm1RLJV9LwxPusT6TNPRW7hhXpZp+MWCTejeEHK+jXzEV2fPG7K8FeM0T5 +VSe0Q75wFZrTYdGjQMPdn5gwof6I4YKCpU435gqR00ZaqAppSBmolZZjVZhXhuvb/+XlaZ8ItPh7 N0EB9XpBLEtdXDfqAQ+JgIK6dQsAEEchTKmTlcsKRQEe5O0AoLfdCXWT5n56GdnaFzYIhbgjinXFQ DfdcrEKj0GBqh4dCo8MKxx+tK0bTGcQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1ePupN-0005Xx-Mg; Fri, 15 Dec 2017 18:35:21 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1ePupF-0004J5-J0 for linux-arm-kernel@lists.infradead.org; Fri, 15 Dec 2017 18:35:19 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6C2EA1435; Fri, 15 Dec 2017 10:34:47 -0800 (PST) Received: from e103592.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id A21983F318; Fri, 15 Dec 2017 10:34:46 -0800 (PST) From: Dave Martin To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] arm64: fpsimd: Fix state leakage when migrating after sigreturn Date: Fri, 15 Dec 2017 18:34:38 +0000 Message-Id: <1513362878-8787-1-git-send-email-Dave.Martin@arm.com> X-Mailer: git-send-email 2.1.4 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20171215_103513_695572_C00B996F X-CRM114-Status: GOOD ( 16.67 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Geert Uytterhoeven , Will Deacon , Ard Biesheuvel MIME-Version: 1.0 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP When refactoring the sigreturn code to handle SVE, I changed the sigreturn implementation to store the new FPSIMD state from the user sigframe into task_struct before reloading the state into the CPU regs. This makes it easier to convert the data for SVE when needed. However, it turns out that the fpsimd_state structure passed into fpsimd_update_current_state is not fully initialised, so assigning the structure as a whole corrupts current->thread.fpsimd_state.cpu with uninitialised data. This means that if the garbage data written to .cpu happens to be a valid cpu number, and the task is subsequently migrated to the cpu identified by the that number, and then tries to enter userspace, the CPU FPSIMD regs will be assumed to be correct for the task and not reloaded as they should be. This can result in returning to userspace with the FPSIMD registers containing data that is stale or that belongs to another task or to the kernel. Knowingly handing around a kernel structure that is incompletely initialised with user data is a potential source of mistakes, especially across source file boundaries. To help avoid a repeat of this issue, this patch adapts the relevant internal API to hand around the user-accessible subset only: struct user_fpsimd_state. To avoid future surprises, this patch also converts all uses of struct fpsimd_state that really only access the user subset, to use struct user_fpsimd_state. A few missing consts are added to function prototypes for good measure. Thanks to Will for spotting the cause of the bug here. Reported-by: Geert Uytterhoeven Fixes: 8cd969d28fd2 ("arm64/sve: Signal handling support") Signed-off-by: Dave Martin Cc: Will Deacon Cc: Ard Biesheuvel --- This can be applied either on top of Will's point fix, or separately. The rebase on top should be pretty trivial. arch/arm64/include/asm/fpsimd.h | 2 +- arch/arm64/kernel/fpsimd.c | 4 ++-- arch/arm64/kernel/signal.c | 7 ++++--- arch/arm64/kernel/signal32.c | 5 +++-- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h index 74f3439..8857a0f 100644 --- a/arch/arm64/include/asm/fpsimd.h +++ b/arch/arm64/include/asm/fpsimd.h @@ -71,7 +71,7 @@ extern void fpsimd_flush_thread(void); extern void fpsimd_signal_preserve_current_state(void); extern void fpsimd_preserve_current_state(void); extern void fpsimd_restore_current_state(void); -extern void fpsimd_update_current_state(struct fpsimd_state *state); +extern void fpsimd_update_current_state(struct user_fpsimd_state const *state); extern void fpsimd_flush_task_state(struct task_struct *target); extern void sve_flush_cpu_state(void); diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 540a1e0..55fb544 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -1036,14 +1036,14 @@ void fpsimd_restore_current_state(void) * flag that indicates that the FPSIMD register contents are the most recent * FPSIMD state of 'current' */ -void fpsimd_update_current_state(struct fpsimd_state *state) +void fpsimd_update_current_state(struct user_fpsimd_state const *state) { if (!system_supports_fpsimd()) return; local_bh_disable(); - current->thread.fpsimd_state = *state; + current->thread.fpsimd_state.user_fpsimd = *state; if (system_supports_sve() && test_thread_flag(TIF_SVE)) fpsimd_to_sve(current); diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index b120111..f60c052 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -178,7 +178,8 @@ static void __user *apply_user_offset( static int preserve_fpsimd_context(struct fpsimd_context __user *ctx) { - struct fpsimd_state *fpsimd = ¤t->thread.fpsimd_state; + struct user_fpsimd_state const *fpsimd = + ¤t->thread.fpsimd_state.user_fpsimd; int err; /* copy the FP and status/control registers */ @@ -195,7 +196,7 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx) static int restore_fpsimd_context(struct fpsimd_context __user *ctx) { - struct fpsimd_state fpsimd; + struct user_fpsimd_state fpsimd; __u32 magic, size; int err = 0; @@ -266,7 +267,7 @@ static int restore_sve_fpsimd_context(struct user_ctxs *user) { int err; unsigned int vq; - struct fpsimd_state fpsimd; + struct user_fpsimd_state fpsimd; struct sve_context sve; if (__copy_from_user(&sve, user->sve, sizeof(sve))) diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c index 22711ee..a124140 100644 --- a/arch/arm64/kernel/signal32.c +++ b/arch/arm64/kernel/signal32.c @@ -228,7 +228,8 @@ union __fpsimd_vreg { static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame) { - struct fpsimd_state *fpsimd = ¤t->thread.fpsimd_state; + struct user_fpsimd_state const *fpsimd = + ¤t->thread.fpsimd_state.user_fpsimd; compat_ulong_t magic = VFP_MAGIC; compat_ulong_t size = VFP_STORAGE_SIZE; compat_ulong_t fpscr, fpexc; @@ -277,7 +278,7 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame) static int compat_restore_vfp_context(struct compat_vfp_sigframe __user *frame) { - struct fpsimd_state fpsimd; + struct user_fpsimd_state fpsimd; compat_ulong_t magic = VFP_MAGIC; compat_ulong_t size = VFP_STORAGE_SIZE; compat_ulong_t fpscr;