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=-8.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham 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 75470C43461 for ; Mon, 14 Sep 2020 14:50:59 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2057E207EA for ; Mon, 14 Sep 2020 14:50:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2057E207EA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 819B06B0037; Mon, 14 Sep 2020 10:50:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7CA426B0062; Mon, 14 Sep 2020 10:50:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6B7D16B006C; Mon, 14 Sep 2020 10:50:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 50A446B0037 for ; Mon, 14 Sep 2020 10:50:58 -0400 (EDT) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id F35991DF7 for ; Mon, 14 Sep 2020 14:50:57 +0000 (UTC) X-FDA: 77261954196.12.cream79_18162b727109 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin12.hostedemail.com (Postfix) with ESMTP id 69AA618005A70 for ; Mon, 14 Sep 2020 14:50:57 +0000 (UTC) X-HE-Tag: cream79_18162b727109 X-Filterd-Recvd-Size: 10249 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by imf42.hostedemail.com (Postfix) with ESMTP for ; Mon, 14 Sep 2020 14:50:55 +0000 (UTC) IronPort-SDR: aw6vbC1Ivlf+urL352n0G0OSaC1rSLu0UncDiKEYyZFqzmyw2J64K1VtaeiCEf9NDqG6wH0M32 /x6U1pO+KYgA== X-IronPort-AV: E=McAfee;i="6000,8403,9744"; a="139102025" X-IronPort-AV: E=Sophos;i="5.76,426,1592895600"; d="scan'208";a="139102025" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2020 07:50:53 -0700 IronPort-SDR: D22eyFJtL3FUI50v72FdSc2QGREc8o8ZWP6ylwCDYTERMIczAE9TLj/D9Mh+x6jWZTQK0s+dVI Xf7HiQv3zKCw== X-IronPort-AV: E=Sophos;i="5.76,426,1592895600"; d="scan'208";a="507168511" Received: from pkamat-mobl1.amr.corp.intel.com (HELO [10.209.79.20]) ([10.209.79.20]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Sep 2020 07:50:49 -0700 Subject: Re: [NEEDS-REVIEW] Re: [PATCH v11 25/25] x86/cet/shstk: Add arch_prctl functions for shadow stack To: Yu-cheng Yu , Andy Lutomirski Cc: Dave Martin , "H.J. Lu" , Florian Weimer , 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 , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , "Ravi V. Shankar" , Vedvyas Shanbhogue , Weijiang Yang References: <086c73d8-9b06-f074-e315-9964eb666db9@intel.com> <0e9996bc-4c1b-cc99-9616-c721b546f857@intel.com> <4f2dfefc-b55e-bf73-f254-7d95f9c67e5c@intel.com> <20200901102758.GY6642@arm.com> <32005d57-e51a-7c7f-4e86-612c2ff067f3@intel.com> <46dffdfd-92f8-0f05-6164-945f217b0958@intel.com> <6e1e22a5-1b7f-2783-351e-c8ed2d4893b8@intel.com> <5979c58d-a6e3-d14d-df92-72cdeb97298d@intel.com> <08c91835-8486-9da5-a7d1-75e716fc5d36@intel.com> From: Dave Hansen Autocrypt: addr=dave.hansen@intel.com; keydata= xsFNBE6HMP0BEADIMA3XYkQfF3dwHlj58Yjsc4E5y5G67cfbt8dvaUq2fx1lR0K9h1bOI6fC oAiUXvGAOxPDsB/P6UEOISPpLl5IuYsSwAeZGkdQ5g6m1xq7AlDJQZddhr/1DC/nMVa/2BoY 2UnKuZuSBu7lgOE193+7Uks3416N2hTkyKUSNkduyoZ9F5twiBhxPJwPtn/wnch6n5RsoXsb ygOEDxLEsSk/7eyFycjE+btUtAWZtx+HseyaGfqkZK0Z9bT1lsaHecmB203xShwCPT49Blxz VOab8668QpaEOdLGhtvrVYVK7x4skyT3nGWcgDCl5/Vp3TWA4K+IofwvXzX2ON/Mj7aQwf5W iC+3nWC7q0uxKwwsddJ0Nu+dpA/UORQWa1NiAftEoSpk5+nUUi0WE+5DRm0H+TXKBWMGNCFn c6+EKg5zQaa8KqymHcOrSXNPmzJuXvDQ8uj2J8XuzCZfK4uy1+YdIr0yyEMI7mdh4KX50LO1 pmowEqDh7dLShTOif/7UtQYrzYq9cPnjU2ZW4qd5Qz2joSGTG9eCXLz5PRe5SqHxv6ljk8mb ApNuY7bOXO/A7T2j5RwXIlcmssqIjBcxsRRoIbpCwWWGjkYjzYCjgsNFL6rt4OL11OUF37wL QcTl7fbCGv53KfKPdYD5hcbguLKi/aCccJK18ZwNjFhqr4MliQARAQABzShEYXZpZCBDaHJp c3RvcGhlciBIYW5zZW4gPGRhdmVAc3I3MS5uZXQ+wsF7BBMBAgAlAhsDBgsJCAcDAgYVCAIJ CgsEFgIDAQIeAQIXgAUCTo3k0QIZAQAKCRBoNZUwcMmSsMO2D/421Xg8pimb9mPzM5N7khT0 2MCnaGssU1T59YPE25kYdx2HntwdO0JA27Wn9xx5zYijOe6B21ufrvsyv42auCO85+oFJWfE K2R/IpLle09GDx5tcEmMAHX6KSxpHmGuJmUPibHVbfep2aCh9lKaDqQR07gXXWK5/yU1Dx0r VVFRaHTasp9fZ9AmY4K9/BSA3VkQ8v3OrxNty3OdsrmTTzO91YszpdbjjEFZK53zXy6tUD2d e1i0kBBS6NLAAsqEtneplz88T/v7MpLmpY30N9gQU3QyRC50jJ7LU9RazMjUQY1WohVsR56d ORqFxS8ChhyJs7BI34vQusYHDTp6PnZHUppb9WIzjeWlC7Jc8lSBDlEWodmqQQgp5+6AfhTD kDv1a+W5+ncq+Uo63WHRiCPuyt4di4/0zo28RVcjtzlGBZtmz2EIC3vUfmoZbO/Gn6EKbYAn rzz3iU/JWV8DwQ+sZSGu0HmvYMt6t5SmqWQo/hyHtA7uF5Wxtu1lCgolSQw4t49ZuOyOnQi5 f8R3nE7lpVCSF1TT+h8kMvFPv3VG7KunyjHr3sEptYxQs4VRxqeirSuyBv1TyxT+LdTm6j4a mulOWf+YtFRAgIYyyN5YOepDEBv4LUM8Tz98lZiNMlFyRMNrsLV6Pv6SxhrMxbT6TNVS5D+6 UorTLotDZKp5+M7BTQRUY85qARAAsgMW71BIXRgxjYNCYQ3Xs8k3TfAvQRbHccky50h99TUY sqdULbsb3KhmY29raw1bgmyM0a4DGS1YKN7qazCDsdQlxIJp9t2YYdBKXVRzPCCsfWe1dK/q 66UVhRPP8EGZ4CmFYuPTxqGY+dGRInxCeap/xzbKdvmPm01Iw3YFjAE4PQ4hTMr/H76KoDbD cq62U50oKC83ca/PRRh2QqEqACvIH4BR7jueAZSPEDnzwxvVgzyeuhwqHY05QRK/wsKuhq7s UuYtmN92Fasbxbw2tbVLZfoidklikvZAmotg0dwcFTjSRGEg0Gr3p/xBzJWNavFZZ95Rj7Et db0lCt0HDSY5q4GMR+SrFbH+jzUY/ZqfGdZCBqo0cdPPp58krVgtIGR+ja2Mkva6ah94/oQN lnCOw3udS+Eb/aRcM6detZr7XOngvxsWolBrhwTQFT9D2NH6ryAuvKd6yyAFt3/e7r+HHtkU kOy27D7IpjngqP+b4EumELI/NxPgIqT69PQmo9IZaI/oRaKorYnDaZrMXViqDrFdD37XELwQ gmLoSm2VfbOYY7fap/AhPOgOYOSqg3/Nxcapv71yoBzRRxOc4FxmZ65mn+q3rEM27yRztBW9 AnCKIc66T2i92HqXCw6AgoBJRjBkI3QnEkPgohQkZdAb8o9WGVKpfmZKbYBo4pEAEQEAAcLB XwQYAQIACQUCVGPOagIbDAAKCRBoNZUwcMmSsJeCEACCh7P/aaOLKWQxcnw47p4phIVR6pVL e4IEdR7Jf7ZL00s3vKSNT+nRqdl1ugJx9Ymsp8kXKMk9GSfmZpuMQB9c6io1qZc6nW/3TtvK pNGz7KPPtaDzvKA4S5tfrWPnDr7n15AU5vsIZvgMjU42gkbemkjJwP0B1RkifIK60yQqAAlT YZ14P0dIPdIPIlfEPiAWcg5BtLQU4Wg3cNQdpWrCJ1E3m/RIlXy/2Y3YOVVohfSy+4kvvYU3 lXUdPb04UPw4VWwjcVZPg7cgR7Izion61bGHqVqURgSALt2yvHl7cr68NYoFkzbNsGsye9ft M9ozM23JSgMkRylPSXTeh5JIK9pz2+etco3AfLCKtaRVysjvpysukmWMTrx8QnI5Nn5MOlJj 1Ov4/50JY9pXzgIDVSrgy6LYSMc4vKZ3QfCY7ipLRORyalFDF3j5AGCMRENJjHPD6O7bl3Xo 4DzMID+8eucbXxKiNEbs21IqBZbbKdY1GkcEGTE7AnkA3Y6YB7I/j9mQ3hCgm5muJuhM/2Fr OPsw5tV/LmQ5GXH0JQ/TZXWygyRFyyI2FqNTx4WHqUn3yFj8rwTAU1tluRUYyeLy0ayUlKBH ybj0N71vWO936MqP6haFERzuPAIpxj2ezwu0xb1GjTk4ynna6h5GjnKgdfOWoRtoWndMZxbA z5cecg== Message-ID: <41aa5e8f-ad88-2934-6d10-6a78fcbe019b@intel.com> Date: Mon, 14 Sep 2020 07:50:49 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US X-Rspamd-Queue-Id: 69AA618005A70 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam04 Content-Transfer-Encoding: quoted-printable 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 9/11/20 3:59 PM, Yu-cheng Yu wrote: ... > Here are the changes if we take the mprotect(PROT_SHSTK) approach. > Any comments/suggestions? I still don't like it. :) I'll also be much happier when there's a proper changelog to accompany this which also spells out the alternatives any why they suck so much. > diff --git a/arch/x86/include/uapi/asm/mman.h b/arch/x86/include/uapi/a= sm/mman.h > index d4a8d0424bfb..024f006fcfe8 100644 > --- a/arch/x86/include/uapi/asm/mman.h > +++ b/arch/x86/include/uapi/asm/mman.h > @@ -4,6 +4,8 @@ > =20 > #define MAP_32BIT 0x40 /* only give out 32bit addresses */ > =20 > +#define PROT_SHSTK 0x10 /* shadow stack pages */ > + > #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS > /* > * Take the 4 protection key bits out of the vma->vm_flags > @@ -19,13 +21,35 @@ > ((vm_flags) & VM_PKEY_BIT2 ? _PAGE_PKEY_BIT2 : 0) | \ > ((vm_flags) & VM_PKEY_BIT3 ? _PAGE_PKEY_BIT3 : 0)) > =20 > -#define arch_calc_vm_prot_bits(prot, key) ( \ > +#define pkey_vm_prot_bits(prot, key) ( \ > ((key) & 0x1 ? VM_PKEY_BIT0 : 0) | \ > ((key) & 0x2 ? VM_PKEY_BIT1 : 0) | \ > ((key) & 0x4 ? VM_PKEY_BIT2 : 0) | \ > ((key) & 0x8 ? VM_PKEY_BIT3 : 0)) > +#else > +#define pkey_vm_prot_bits(prot, key) > #endif My inner compiler doesn't think this will compile: ( | shstk_vm_prot_bits(prot)) > +#define shstk_vm_prot_bits(prot) ( \ > + (static_cpu_has(X86_FEATURE_SHSTK) && (prot & PROT_SHSTK)) ? \ > + VM_SHSTK : 0) Why do you need to filter PROT_SHSTK twice. Won't the prot passed in here be filtered by arch_validate_prot()? > +#define arch_calc_vm_prot_bits(prot, key) \ > + (pkey_vm_prot_bits(prot, key) | shstk_vm_prot_bits(prot)) > + IMNHO, this is eminently more readable if you do: #define arch_calc_vm_prot_bits(prot, key) \ (shstk_vm_prot_bits(prot)) \ pkey_vm_prot_bits(prot, key)) BTW, can these be static inlines? I forget if I had a good reason for making them #defines. > +static inline bool arch_validate_prot(unsigned long prot, unsigned lon= g addr) > +{ > + unsigned long supported =3D PROT_READ | PROT_EXEC | PROT_SEM; > + > + if (static_cpu_has(X86_FEATURE_SHSTK) && (prot & PROT_SHSTK)) > + supported |=3D PROT_SHSTK; > + else > + supported |=3D PROT_WRITE; I generally like to make the common case dirt simple to understand. That would probably be: unsigned long supported =3D PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM; if (static_cpu_has(X86_FEATURE_SHSTK) && (prot & PROT_SHSTK)) { supported |=3D PROT_SHSTK; // Comment about why SHSTK and WRITE // are mutually exclusive. supported &=3D ~PROT_WRITE; } > #endif /* _ASM_X86_MMAN_H */ > diff --git a/mm/mprotect.c b/mm/mprotect.c > index a8edbcb3af99..520bd8caa005 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -571,6 +571,17 @@ static int do_mprotect_pkey(unsigned long start, s= ize_t > len, > goto out; > } > } > + > + /* > + * Only anonymous mapping is suitable for shadow stack. > + */ Why? > + if (prot & PROT_SHSTK) { > + if (vma->vm_file) { > + error =3D -EINVAL; > + goto out; > + } > + } You can also save a couple of lines there. The two conditions are pretty small.