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 EB614C433F5 for ; Mon, 3 Oct 2022 22:23:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8577A6B0071; Mon, 3 Oct 2022 18:23:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 806756B0073; Mon, 3 Oct 2022 18:23:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6A77A6B0074; Mon, 3 Oct 2022 18:23:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 570076B0071 for ; Mon, 3 Oct 2022 18:23:56 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 24B36806D1 for ; Mon, 3 Oct 2022 22:23:56 +0000 (UTC) X-FDA: 79981066872.01.21C73AD Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) by imf05.hostedemail.com (Postfix) with ESMTP id 9412710000E for ; Mon, 3 Oct 2022 22:23:55 +0000 (UTC) Received: by mail-pj1-f41.google.com with SMTP id h8-20020a17090a054800b00205ccbae31eso16541935pjf.5 for ; Mon, 03 Oct 2022 15:23:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=z1/2plwrXjBAn1kAQlG8HZaZ4O1cAMi4uWpIyecb3WY=; b=fL7uVv5uocG4gFvRiRzu57UBR+Y+hOjSt1rMC9dMipp0dZgJYkPj5efzAvlOD26mpe f++eXumz+X91j0ZagKBWZTHo8Kl7b3zELSamfhfgQ+F7k426k2hwkZHBxmeLHXs8rtPR aGOXykeUS0F0d6SIzP4G+ZZ+bPrCLDt8t7s+8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=z1/2plwrXjBAn1kAQlG8HZaZ4O1cAMi4uWpIyecb3WY=; b=qSOrniMlAdmMtVv/vJMemk0Uj3lAcceDpHSNQmfYMrrWxAxcSzoFNg/QXSKsJ9HlCR nMbOp0dZvPnXyo3TqiFUSgDzoaAzOWBPkpo8TXj1ryQFstHQ58DNpkta1+V3LVi7WnGB M7ceU47ZFSAaq/styx4WH4cps3Foehv4FdYI2XI2rQFDdd9cFa1gN0o/o3LL9j9X7T7I fQ/aq6l1JArRA2n+eAgWGtTDodXY2lxcGqxxXf0itqEu9CGmZaxr1ohjXKw+v/VwZvz+ uT9lXd/08JJP+8BvDCc81eCaTo4lA6S6v1fxDG9UUfFc05cdbyhZOubH+yKtmxtBtU0D 1Z8A== X-Gm-Message-State: ACrzQf3AIeLpYD3L/8nRAvNdBsltSHXsS8lB4icjJRkTPVADd6yZLgoA kSL1z8hZs+xADg8fD0Jmy/aZKg== X-Google-Smtp-Source: AMsMyM5T8uqzbmlhDwK2iJakHiD/I/rnTl7wYc9IoQSun+00KcqIfUGmuustAbFdNeZC1kbM95GA+A== X-Received: by 2002:a17:90b:3852:b0:203:a68c:9a7c with SMTP id nl18-20020a17090b385200b00203a68c9a7cmr14529200pjb.119.1664835834467; Mon, 03 Oct 2022 15:23:54 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id n7-20020a170902e54700b00174f61a7d09sm7713150plf.247.2022.10.03.15.23.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Oct 2022 15:23:53 -0700 (PDT) Date: Mon, 3 Oct 2022 15:23:52 -0700 From: Kees Cook To: Rick Edgecombe Cc: x86@kernel.org, "H . Peter Anvin" , Thomas Gleixner , Ingo Molnar , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org, Arnd Bergmann , Andy Lutomirski , Balbir Singh , Borislav Petkov , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H . J . Lu" , Jann Horn , Jonathan Corbet , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , "Ravi V . Shankar" , Weijiang Yang , "Kirill A . Shutemov" , joao.moreira@intel.com, John Allen , kcc@google.com, eranian@google.com, rppt@kernel.org, jamorris@linux.microsoft.com, dethoma@microsoft.com Subject: Re: [PATCH v2 28/39] x86/cet/shstk: Introduce map_shadow_stack syscall Message-ID: <202210031446.E4AD9EE66@keescook> References: <20220929222936.14584-1-rick.p.edgecombe@intel.com> <20220929222936.14584-29-rick.p.edgecombe@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220929222936.14584-29-rick.p.edgecombe@intel.com> ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1664835835; 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=z1/2plwrXjBAn1kAQlG8HZaZ4O1cAMi4uWpIyecb3WY=; b=wL1C3RvvAu9Ly+IMGst/Okbe9iHchbm9iXuWZo4yTwWj+Jr7ltbZPZNR7lt76zNF6qHlT8 8qOD4+9z19faCzLcdb73XHWuuP99S89lLql7w5v6fAlCWBts5XQYAsWGrJCgVuyLQvfLiJ wDVjf+VUwsOZnvDc/mSh1rGJSR+W7gg= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=fL7uVv5u; spf=pass (imf05.hostedemail.com: domain of keescook@chromium.org designates 209.85.216.41 as permitted sender) smtp.mailfrom=keescook@chromium.org; dmarc=pass (policy=none) header.from=chromium.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1664835835; a=rsa-sha256; cv=none; b=nIET2tZafPJAQi7XeuH1GKUvhSHMcnlQ8ZeUVWXRCUZb1Uw9mArEmrMv3obEWdw5N0xMTi S3a5JR2q2vvaesMKKMb6u+KpDg/sQLxE2TFGObhbXqx7X2CIUlc0okh9tPl9nCNR/++pn7 Tkue+WpkpRN3+4YNHqd0zXqudBBj8F0= X-Rspam-User: Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=fL7uVv5u; spf=pass (imf05.hostedemail.com: domain of keescook@chromium.org designates 209.85.216.41 as permitted sender) smtp.mailfrom=keescook@chromium.org; dmarc=pass (policy=none) header.from=chromium.org X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 9412710000E X-Stat-Signature: 5sx4g4u6jcgbaim5njgaqpqzzxikw8rn X-HE-Tag: 1664835835-983800 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 Thu, Sep 29, 2022 at 03:29:25PM -0700, Rick Edgecombe wrote: > [...] > The following example demonstrates how to create a new shadow stack with > map_shadow_stack: > void *shstk = map_shadow_stack(adrr, stack_size, SHADOW_STACK_SET_TOKEN); typo: addr > [...] > +451 common map_shadow_stack sys_map_shadow_stack Isn't this "64", not "common"? > [...] > +#define SHADOW_STACK_SET_TOKEN 0x1 /* Set up a restore token in the shadow stack */ I think this should get an intro comment, like: /* Flags for map_shadow_stack(2) */ Also, as with the other UAPI fields, please use "(1ULL << 0)" here. > @@ -62,24 +63,34 @@ static int create_rstor_token(unsigned long ssp, unsigned long *token_addr) > if (write_user_shstk_64((u64 __user *)addr, (u64)ssp)) > return -EFAULT; > > - *token_addr = addr; > + if (token_addr) > + *token_addr = addr; > > return 0; > } > Can this just be collapsed into the patch that introduces create_rstor_token()? > -static unsigned long alloc_shstk(unsigned long size) > +static unsigned long alloc_shstk(unsigned long addr, unsigned long size, > + unsigned long token_offset, bool set_res_tok) > { > int flags = MAP_ANONYMOUS | MAP_PRIVATE; > struct mm_struct *mm = current->mm; > - unsigned long addr, unused; > + unsigned long mapped_addr, unused; > > mmap_write_lock(mm); > - addr = do_mmap(NULL, addr, size, PROT_READ, flags, Oops, I missed in the other patch that "addr" was being passed here. (uninitialized?) > - VM_SHADOW_STACK | VM_WRITE, 0, &unused, NULL); > - > + mapped_addr = do_mmap(NULL, addr, size, PROT_READ, flags, > + VM_SHADOW_STACK | VM_WRITE, 0, &unused, NULL); I don't see do_mmap() doing anything here to avoid remapping a prior vma as shstk. Is the intention to allow userspace to convert existing VMAs? This has caused pain in the past, perhaps force MAP_FIXED_NOREPLACE ? > [...] > @@ -174,6 +185,7 @@ int shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags, > > > stack_size = PAGE_ALIGN(stack_size); > + addr = alloc_shstk(0, stack_size, 0, false); > if (IS_ERR_VALUE(addr)) > return PTR_ERR((void *)addr); > As mentioned earlier, I was expecting this patch to replace a (missing) call to alloc_shstk. i.e. expecting: - addr = alloc_shstk(stack_size); > @@ -395,6 +407,26 @@ int shstk_disable(void) > return 0; > } > > + > +SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags) Please add kern-doc for this, with some notes. E.g. at least one thing isn't immediately obvious, maybe more: "addr" must be a multiple of 8. > +{ > + unsigned long aligned_size; > + > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK)) > + return -ENOSYS; This needs to explicitly reject unknown flags[1], or expanding them in the future becomes very painful: if (flags & ~(SHADOW_STACK_SET_TOKEN)) return -EINVAL; [1] https://docs.kernel.org/process/adding-syscalls.html#designing-the-api-planning-for-extension > + > + /* > + * An overflow would result in attempting to write the restore token > + * to the wrong location. Not catastrophic, but just return the right > + * error code and block it. > + */ > + aligned_size = PAGE_ALIGN(size); > + if (aligned_size < size) > + return -EOVERFLOW; The intention here is to allow userspace to ask for _less_ than a page size multiple, and to put the restore token there? Is it worth adding a check for size >= 8 here? Or, I guess it would just immediately crash on the next call? > + > + return alloc_shstk(addr, aligned_size, size, flags & SHADOW_STACK_SET_TOKEN); > +} -- Kees Cook