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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C27EC61DA4 for ; Thu, 9 Mar 2023 14:12:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 651B8280002; Thu, 9 Mar 2023 09:12:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 60124280001; Thu, 9 Mar 2023 09:12:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4A1F5280002; Thu, 9 Mar 2023 09:12:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3B0A9280001 for ; Thu, 9 Mar 2023 09:12:44 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 3E92C1C6920 for ; Thu, 9 Mar 2023 14:12:41 +0000 (UTC) X-FDA: 80549550522.22.152DA36 Received: from mail.skyhub.de (mail.skyhub.de [5.9.137.197]) by imf07.hostedemail.com (Postfix) with ESMTP id 3219440006 for ; Thu, 9 Mar 2023 14:12:35 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=temperror ("DNS error when getting key") header.d=alien8.de header.s=dkim header.b=daTlHMXW; spf=pass (imf07.hostedemail.com: domain of bp@alien8.de designates 5.9.137.197 as permitted sender) smtp.mailfrom=bp@alien8.de; dmarc=pass (policy=none) header.from=alien8.de ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678371159; a=rsa-sha256; cv=none; b=zWvRQK1pRe0Sei0Kz8KqZtdFAISzSzi91NKKWjOLLmqwOoxgV5dUaB7dXUsvAd1YndbFZf fGQEFpRz51EBxhdXsOlKg889+A6Pe9gZhcHiCYENeVJpKeVtkqq6HGDcUJ4V99SLY9UaQ3 WiBMlt3u6OhIaWHlOSkxsDdzPkby+ps= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=temperror ("DNS error when getting key") header.d=alien8.de header.s=dkim header.b=daTlHMXW; spf=pass (imf07.hostedemail.com: domain of bp@alien8.de designates 5.9.137.197 as permitted sender) smtp.mailfrom=bp@alien8.de; dmarc=pass (policy=none) header.from=alien8.de ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678371159; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=w2fyPKd/3AyQO0vRIYAONySJuiNLLI9cVCZnriSsS1Y=; b=66DPcubWM3Ic8qTDSqZUSPY51L7pbLAAV9k5liDGYo0JOmD2wsYiYuFslAAkKRQAGK1W5g 4C5XvBfvAU1YdOPX9uXKccUJ6vf7tZtpJas2H6IxOMyYBnL/oTqKP4AaLhZEvkKIvQu/FY mS1FEbqc2oB5NWtBxhQllNS/XVbzOtQ= Received: from zn.tnic (p5de8e9fe.dip0.t-ipconnect.de [93.232.233.254]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.skyhub.de (SuperMail on ZX Spectrum 128k) with ESMTPSA id DBDBE1EC06C0; Thu, 9 Mar 2023 15:12:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alien8.de; s=dkim; t=1678371153; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:in-reply-to:in-reply-to: references:references; bh=w2fyPKd/3AyQO0vRIYAONySJuiNLLI9cVCZnriSsS1Y=; b=daTlHMXWbU7km2++Pqh1oYE/nsYUwrFU5fXZAkkWykVSRndcnEPsQVIjbBxgSOZO1YC0BR 6UsZoKQ7vh+Hyd4HmhRuLraKwnWiCZMKlmf1iY9ezyPRnbOPeO9bzBsWJ/F8t4tRgCvm4g Ol1SfaLsjAlGQdngAX8UoQnSwNF0pb4= Date: Thu, 9 Mar 2023 15:12:29 +0100 From: Borislav Petkov To: "Edgecombe, Rick P" Cc: "david@redhat.com" , "bsingharora@gmail.com" , "hpa@zytor.com" , "Syromiatnikov, Eugene" , "peterz@infradead.org" , "rdunlap@infradead.org" , "keescook@chromium.org" , "Yu, Yu-cheng" , "dave.hansen@linux.intel.com" , "kirill.shutemov@linux.intel.com" , "Eranian, Stephane" , "linux-mm@kvack.org" , "fweimer@redhat.com" , "nadav.amit@gmail.com" , "jannh@google.com" , "dethoma@microsoft.com" , "kcc@google.com" , "linux-arch@vger.kernel.org" , "pavel@ucw.cz" , "oleg@redhat.com" , "hjl.tools@gmail.com" , "Yang, Weijiang" , "Lutomirski, Andy" , "linux-doc@vger.kernel.org" , "arnd@arndb.de" , "tglx@linutronix.de" , "Schimpe, Christina" , "mike.kravetz@oracle.com" , "x86@kernel.org" , "akpm@linux-foundation.org" , "debug@rivosinc.com" , "jamorris@linux.microsoft.com" , "john.allen@amd.com" , "rppt@kernel.org" , "andrew.cooper3@citrix.com" , "mingo@redhat.com" , "corbet@lwn.net" , "linux-kernel@vger.kernel.org" , "linux-api@vger.kernel.org" , "gorcunov@gmail.com" Subject: Re: [PATCH v7 30/41] x86/shstk: Handle thread shadow stack Message-ID: References: <20230227222957.24501-1-rick.p.edgecombe@intel.com> <20230227222957.24501-31-rick.p.edgecombe@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Queue-Id: 3219440006 X-Rspamd-Server: rspam01 X-Stat-Signature: qjd9etbqzbnregorksedr1nisppb74b4 X-HE-Tag: 1678371155-913503 X-HE-Meta: U2FsdGVkX1/20XBrJb63zqR/arT7uEbXzxUfeDO+8SlreA/kAwTqkb9iZrIcVUt/09YVg8sB7ri7F4RFiOys9f6dUVQLxmg/7r8VD/HEu+V7fWGENGYQz9aRA30lZX9huQmNc4ZQHjFmYA4vTXSotPse5t2l5JsdSasT+lK9VLZ0sAvNeT4ZynEdRTTkpgviEihdildlnKY4psOUQI3NgZfI8Pn21AmR8YyiWYZln2H0r8myLXLm8qSsMzSRkUGGdwmm6gA6vJf5n2ZERQ7yQBCwU9AYZtILtZ9MR7WGfKbiLznuDptGdd8HuAIsVNHmTaEf0Qcf0nFG4J875RURFKNvV2kuiNdLMZyeE15JbgkHSzZRgsayBcoapAlVF5vixw0yibWPL0K+BYYHFXoaCS2KTFhtpqQPwQ6CUx5pvvQiSorI6GXQ9dwfrBvZMAGGkDnIeoPmPltw/UcA3q7dxdQD3LatHaJUh0Bs9TXlR8yUqlSn8qmvo+QrsqRvgzUlP7yTMfWjzM6DLxgZiwCs8p2wysztfLcdX8CiPs+ReQkJuu+rsw7Klimp3xSOE/XBXJHpDlzX/vYm5qkIum1dVbu6jZVK3IItAJLJhTiFgYZcch2aAfhtVvQzUXIEcP4SUCRSQ2w/eKXXY1u31UEeqptCEQUGkn26rPnc9UnhFbbM7uDTyle6SmTECQBC14HEAhkIuzWgfptUP1vvIBcmX7rtQedO8DpptEK/fqTKvcgXCM7Bp+WPQpdeWtdOLOPjzAYqE7RiJrUppAdeLNXHjWi3eJiKqR9nJm5yFpluxGtnYKH/dGwKWeW7tM01VVO+tBED5YRtQsq7VCrnXYM+V+exMDbZq7xxLOlbONyz7HqWO+FuwLZbzgIo0iTmxcZZeMwizwNDkETx4V9ODC4tbDvlm7mUiMeMG2uPZTe4oM8PsCnt6rZlzbSKnsoAcd3mRc8ptafmes523JlhJXz xMQEArZV 7gElKQ/CZiE2yV27G698tzJqoKOaFARr4h5F/3KPsDUO84ChVOHHkotd+KfC5fjWw5ymRr/AkVlXejeqEaZ9JOX245SmNOeLaXh3ngOfilWRgOJQ0EuAHwLKsqKXHILLqA68C0Db1UoCJle89I7s+rFd34hdEwn5wWCQzY1qa1Js8/ZLQnxVu9R5V+hcWGY+Hn36TNhp6AdTjqKtNJdplzPZFTAIi5xOuJRjbtoTl7AmWLrZthG+yZ0hibYbwncNzkz6xKtvPDkA8pPSoBosW+q/5PgrIW1NfNErrqpdU86Adi1h9eu5pAXms5Cl2BIr33HDAbmTSfz7SXWY= 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 Wed, Mar 08, 2023 at 08:03:17PM +0000, Edgecombe, Rick P wrote: Btw, pls try to trim your replies as I need ot scroll through pages of quoted text to find the response. > Sure. Sometimes people tell me to only ifdef out whole functions to > make it easier to read. I suppose in this case it's not hard to see. Yeah, the less ifdeffery we have, the better. > If the default SSP value logic is too hidden, what about some clearer > code and comments, like this? The problem with this function is that it needs to return three things: * success: ** 0 or ** shadow stack address * failure: due to allocation. How about this below instead? (totally untested ofc): diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c index bf703f53fa49..6e323d4e32fc 100644 --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c @@ -142,7 +142,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) struct inactive_task_frame *frame; struct fork_frame *fork_frame; struct pt_regs *childregs; - unsigned long shstk_addr = 0; + unsigned long shstk_addr; int ret = 0; childregs = task_pt_regs(p); @@ -178,10 +178,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args) #endif /* Allocate a new shadow stack for pthread if needed */ - ret = shstk_alloc_thread_stack(p, clone_flags, args->stack_size, - &shstk_addr); - if (ret) - return ret; + shstk_addr = shstk_alloc_thread_stack(p, clone_flags, args->stack_size); + if (IS_ERR_VALUE(shstk_addr)) + return PTR_ERR((void *)shstk_addr); fpu_clone(p, clone_flags, args->fn, shstk_addr); diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c index 13c02747386f..b1668b499e9a 100644 --- a/arch/x86/kernel/shstk.c +++ b/arch/x86/kernel/shstk.c @@ -157,8 +157,8 @@ void reset_thread_features(void) current->thread.features_locked = 0; } -int shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, - unsigned long stack_size, unsigned long *shstk_addr) +unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, + unsigned long stack_size) { struct thread_shstk *shstk = &tsk->thread.shstk; unsigned long addr, size; @@ -180,14 +180,12 @@ int shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, size = adjust_shstk_size(stack_size); addr = alloc_shstk(size); if (IS_ERR_VALUE(addr)) - return PTR_ERR((void *)addr); + return addr; shstk->base = addr; shstk->size = size; - *shstk_addr = addr + size; - - return 0; + return addr + size; } static unsigned long get_user_shstk_addr(void) -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette