From patchwork Thu Dec 2 13:12:50 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tim Gardner X-Patchwork-Id: 1562694 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=canonical.com header.i=@canonical.com header.a=rsa-sha256 header.s=20210705 header.b=jivN+UsQ; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4J4bxJ2flWz9s1l for ; Fri, 3 Dec 2021 00:13:24 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1msltm-0005rZ-9C; Thu, 02 Dec 2021 13:13:18 +0000 Received: from smtp-relay-internal-1.internal ([10.131.114.114] helo=smtp-relay-internal-1.canonical.com) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1msltc-0005mA-KX for kernel-team@lists.ubuntu.com; Thu, 02 Dec 2021 13:13:08 +0000 Received: from mail-pj1-f72.google.com (mail-pj1-f72.google.com [209.85.216.72]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id 6ADDB3F1AE for ; Thu, 2 Dec 2021 13:13:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1638450788; bh=o03a8DOweWyudGi/l/dsacS3c+TwjLiCEctSN8BMpcM=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=jivN+UsQBEExacb9z9Emn36wzPIGvuJJDuDV/GjRHgSGEm4lH/h270TNupj1Palsj oWUeouR4+JR/lUYjQz3GFWNkQgW8dCbZfBoOTH9hTCFOjCtUgRNUs8WTp7VlT37U8d 8bd20uTuzr1+2pEZCg18Qdy+y2xzEYxUiO0YmQqk0A4s66CIUrFYo0kvC0SimL0lnZ AGWXa7IaaQnbSNuL3OYFtuvWLm5cfzJbzCK+I4o2FqlYQDFbrbwTmZ+FzGXvlkzAeI 9VT7779tGkzKhs62noOiC83z26t+O4vKDpEnjnxU9WzM/IaSGwRXh942L+LIe7PdyL Pvze/QYARlQzg== Received: by mail-pj1-f72.google.com with SMTP id b11-20020a17090acc0b00b001a9179dc89fso1947761pju.6 for ; Thu, 02 Dec 2021 05:13:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=o03a8DOweWyudGi/l/dsacS3c+TwjLiCEctSN8BMpcM=; b=yzD8T/hP89CuXYxKkADGbpr1A4sVTcMAJygjDLUAtf15gMbuouKZrE0u1sZAkDEauW chuWiNzrHlzZ0ouWAHGCVYBjgahI4IMQeqgKSBGvOm3euX7gS9LOphPK3XYVoU0ksL2C ZzibNJ9er4Y5F1R3T0tLxZ2fWpEu7X2XtxqF8lBJx1ZVwlMoYHeuyfqUHPqpE4wobito XMlKU3sD1OggZm0zr66J9OtgkSo8XW2bv6jCMUwbg9DMYZySRy8A3fGS58tHhW9RB7em pu72yC6o47oCi9x/r4Yo5uqHa4shhw4UNnk8bvj/9A+cWP8jrbWAYxHzZNGFMwMfuVod n/Yw== X-Gm-Message-State: AOAM532bTuIBUACgr93kIPh2wf0dUSLJYToOhdvzqLUor+WBMJzQTEYV kW2TX0frNHONkbUgYDZddTyCe3S8iQMvnAiB+wdMk+orJ3vw2HddlYeF0LS67hQh6MZGEzh4LUy TllUMe/LjL6NO9v2RmJF+Ce+DjB21ENNXacdRs0qX2Q== X-Received: by 2002:a17:902:ce8f:b0:141:f85a:e0de with SMTP id f15-20020a170902ce8f00b00141f85ae0demr15080597plg.69.1638450786353; Thu, 02 Dec 2021 05:13:06 -0800 (PST) X-Google-Smtp-Source: ABdhPJx3LQ0uNuXA8cplBWKavhkTD2GUb0/r4OJ5HGfpRjc4pE/ZUaV4/UQ1u/f8DBgLCKb1h5BI3Q== X-Received: by 2002:a17:902:ce8f:b0:141:f85a:e0de with SMTP id f15-20020a170902ce8f00b00141f85ae0demr15080567plg.69.1638450786071; Thu, 02 Dec 2021 05:13:06 -0800 (PST) Received: from localhost.localdomain ([69.163.84.166]) by smtp.gmail.com with ESMTPSA id p3sm2666992pjd.45.2021.12.02.05.13.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Dec 2021 05:13:05 -0800 (PST) From: Tim Gardner To: kernel-team@lists.ubuntu.com Subject: [PATCH 02/10] nitro_enclaves: Fix stale file descriptors on failed usercopy Date: Thu, 2 Dec 2021 06:12:50 -0700 Message-Id: <20211202131258.9393-3-tim.gardner@canonical.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20211202131258.9393-1-tim.gardner@canonical.com> References: <20211202131258.9393-1-tim.gardner@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Mathias Krause BugLink: https://bugs.launchpad.net/bugs/1951873 A failing usercopy of the slot uid will lead to a stale entry in the file descriptor table as put_unused_fd() won't release it. This enables userland to refer to a dangling 'file' object through that still valid file descriptor, leading to all kinds of use-after-free exploitation scenarios. Exchanging put_unused_fd() for close_fd(), ksys_close() or alike won't solve the underlying issue, as the file descriptor might have been replaced in the meantime, e.g. via userland calling close() on it (leading to a NULL pointer dereference in the error handling code as 'fget(enclave_fd)' will return a NULL pointer) or by dup2()'ing a completely different file object to that very file descriptor, leading to the same situation: a dangling file descriptor pointing to a freed object -- just in this case to a file object of user's choosing. Generally speaking, after the call to fd_install() the file descriptor is live and userland is free to do whatever with it. We cannot rely on it to still refer to our enclave object afterwards. In fact, by abusing userfaultfd() userland can hit the condition without any racing and abuse the error handling in the nitro code as it pleases. To fix the above issues, defer the call to fd_install() until all possible errors are handled. In this case it's just the usercopy, so do it directly in ne_create_vm_ioctl() itself. Signed-off-by: Mathias Krause Signed-off-by: Andra Paraschiv Cc: stable Link: https://lore.kernel.org/r/20210429165941.27020-2-andraprs@amazon.com Signed-off-by: Greg Kroah-Hartman (cherry picked from commit f1ce3986baa62cffc3c5be156994de87524bab99) Signed-off-by: Tim Gardner --- drivers/virt/nitro_enclaves/ne_misc_dev.c | 43 +++++++++-------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c index f1964ea4b8269..e21e1e86ad15f 100644 --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c @@ -1524,7 +1524,8 @@ static const struct file_operations ne_enclave_fops = { * enclave file descriptor to be further used for enclave * resources handling e.g. memory regions and CPUs. * @ne_pci_dev : Private data associated with the PCI device. - * @slot_uid: Generated unique slot id associated with an enclave. + * @slot_uid: User pointer to store the generated unique slot id + * associated with an enclave to. * * Context: Process context. This function is called with the ne_pci_dev enclave * mutex held. @@ -1532,7 +1533,7 @@ static const struct file_operations ne_enclave_fops = { * * Enclave fd on success. * * Negative return value on failure. */ -static int ne_create_vm_ioctl(struct ne_pci_dev *ne_pci_dev, u64 *slot_uid) +static int ne_create_vm_ioctl(struct ne_pci_dev *ne_pci_dev, u64 __user *slot_uid) { struct ne_pci_dev_cmd_reply cmd_reply = {}; int enclave_fd = -1; @@ -1634,7 +1635,18 @@ static int ne_create_vm_ioctl(struct ne_pci_dev *ne_pci_dev, u64 *slot_uid) list_add(&ne_enclave->enclave_list_entry, &ne_pci_dev->enclaves_list); - *slot_uid = ne_enclave->slot_uid; + if (copy_to_user(slot_uid, &ne_enclave->slot_uid, sizeof(ne_enclave->slot_uid))) { + /* + * As we're holding the only reference to 'enclave_file', fput() + * will call ne_enclave_release() which will do a proper cleanup + * of all so far allocated resources, leaving only the unused fd + * for us to free. + */ + fput(enclave_file); + put_unused_fd(enclave_fd); + + return -EFAULT; + } fd_install(enclave_fd, enclave_file); @@ -1671,34 +1683,13 @@ static long ne_ioctl(struct file *file, unsigned int cmd, unsigned long arg) switch (cmd) { case NE_CREATE_VM: { int enclave_fd = -1; - struct file *enclave_file = NULL; struct ne_pci_dev *ne_pci_dev = ne_devs.ne_pci_dev; - int rc = -EINVAL; - u64 slot_uid = 0; + u64 __user *slot_uid = (void __user *)arg; mutex_lock(&ne_pci_dev->enclaves_list_mutex); - - enclave_fd = ne_create_vm_ioctl(ne_pci_dev, &slot_uid); - if (enclave_fd < 0) { - rc = enclave_fd; - - mutex_unlock(&ne_pci_dev->enclaves_list_mutex); - - return rc; - } - + enclave_fd = ne_create_vm_ioctl(ne_pci_dev, slot_uid); mutex_unlock(&ne_pci_dev->enclaves_list_mutex); - if (copy_to_user((void __user *)arg, &slot_uid, sizeof(slot_uid))) { - enclave_file = fget(enclave_fd); - /* Decrement file refs to have release() called. */ - fput(enclave_file); - fput(enclave_file); - put_unused_fd(enclave_fd); - - return -EFAULT; - } - return enclave_fd; }