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 2CB07C4332F for ; Mon, 30 Oct 2023 20:25:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B59416B0288; Mon, 30 Oct 2023 16:25:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B08F26B0289; Mon, 30 Oct 2023 16:25:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9D0B76B028A; Mon, 30 Oct 2023 16:25:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 8D4146B0288 for ; Mon, 30 Oct 2023 16:25:55 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 558941406DD for ; Mon, 30 Oct 2023 20:25:55 +0000 (UTC) X-FDA: 81403259070.16.693EA93 Received: from mail-yw1-f202.google.com (mail-yw1-f202.google.com [209.85.128.202]) by imf16.hostedemail.com (Postfix) with ESMTP id 8A061180006 for ; Mon, 30 Oct 2023 20:25:53 +0000 (UTC) Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=KXchbFxR; spf=pass (imf16.hostedemail.com: domain of 3UBFAZQYKCKMVHDQMFJRRJOH.FRPOLQXa-PPNYDFN.RUJ@flex--seanjc.bounces.google.com designates 209.85.128.202 as permitted sender) smtp.mailfrom=3UBFAZQYKCKMVHDQMFJRRJOH.FRPOLQXa-PPNYDFN.RUJ@flex--seanjc.bounces.google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1698697553; a=rsa-sha256; cv=none; b=WgaNV2d+f56e+XyvwwSMisNPAWjAbT5ukVnTUiMk8N35+EJlJv9fqZZnD+qYxz0BYaIMWR 12Wnv8CmR8RDYT3yrBq4n96ZdDOM9tPOoyAJdOYPATJX8PYS1N14OzGxM5QX7Z3vCx0qYa PsQ9Bf8txBEIgnmUvsvZLMh2YuQAZa8= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=KXchbFxR; spf=pass (imf16.hostedemail.com: domain of 3UBFAZQYKCKMVHDQMFJRRJOH.FRPOLQXa-PPNYDFN.RUJ@flex--seanjc.bounces.google.com designates 209.85.128.202 as permitted sender) smtp.mailfrom=3UBFAZQYKCKMVHDQMFJRRJOH.FRPOLQXa-PPNYDFN.RUJ@flex--seanjc.bounces.google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1698697553; 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=nLe0geP8e2FVj4qDFmgiKe4HSQmMDr9vIKCdz5IJ0b8=; b=UK81HxZCHCkfqSq1xOYzjQ8IOMPpWNCv8PSescyqdZ4xHAfRzgUoyOfEsoh1TwlgJKLRaY gJV6v6unixTYv8U5PmFG65lbcWR+BMbpuQPd+7jux6PDPe8TtNkwOme74cSDAUgKQEnro/ mhTPGyj6QMVLa4qA3M5vyEIEqmSkX8s= Received: by mail-yw1-f202.google.com with SMTP id 00721157ae682-5acac8b6575so50193657b3.1 for ; Mon, 30 Oct 2023 13:25:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1698697552; x=1699302352; darn=kvack.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=nLe0geP8e2FVj4qDFmgiKe4HSQmMDr9vIKCdz5IJ0b8=; b=KXchbFxRriKc0sEYiyqL7FHVl2p9uywMdkDGnGE3XNHQryKM7tM8uN9bckJoNhq5Td VLTAay1a5Xj6B70cJ30z+aN5qvbwbNf/R0rQRKjV9EAvyVRgcy6bG1hLV9KC+9ndEjJ8 8KikkQC3QESmTgMZKTpzhMr0mJQ/xGYEBnkEA8uCgWsI3njsTOwLFG6ZYns0RfQVzkdq zlD29SUafiSfLUYYCBB8J58vlp6GpHDCfLrOd3GO7zjMjOvajT8VeaODLq05e2JvgfBc adImMWwQF08Ap3FTUQl1fOVMaggvlqU18ujNrdyIKQVlI7CQdnXbEFIgohFHZWhMo/lL +WoA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698697552; x=1699302352; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=nLe0geP8e2FVj4qDFmgiKe4HSQmMDr9vIKCdz5IJ0b8=; b=WI0G3W7ewhauo7vK8LcRhKAPHeGPGwOADaT00YJML5qhcJzWH/SxXceW0SGcAuEDRP BdvO/rpK/SFB1EUMCOdqzPTc4S8HTOT7r8asygOhV8zW26oZuyQWREc66egkW+VdL5d4 7hDW9mHcUOT/WEMyNnPlmZb4y8VX+0XHXqh/Pt7CEEgjlZhVR9pp8uhJzIvu0HP7dlcK 7VNuRyoJULb5qaXcchnILLCCgwVgbQXAthybN9FkaMsuXw8L2xLTJOilBTaMlXyLja8r VaPW6l9XS+PaQ3lr9uJiD9Wk0hB0wBLYu9BwUU054WGOjzed/qSTk2OAb0ovH8wv/2Z5 7xcQ== X-Gm-Message-State: AOJu0Yyx7s6q9rOS6D3HZga6WAuuxhMVPAMgbeCujl/Cupmm6kBaEA3o LIdSPoeCPQfaEvBcl5m/CamqX0xZzgQ= X-Google-Smtp-Source: AGHT+IGlGqmQkLdJGZ9mJzjtHQ/qef61ty+xku12xbQ7aSZ0jjD/v7MM7HUcLrxp67U3zyWvPrrdlUHrpDU= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a25:84cc:0:b0:d89:42d7:e72d with SMTP id x12-20020a2584cc000000b00d8942d7e72dmr15389ybm.3.1698697552520; Mon, 30 Oct 2023 13:25:52 -0700 (PDT) Date: Mon, 30 Oct 2023 13:25:50 -0700 In-Reply-To: <211d093f-4023-4a39-a23f-6d8543512675@redhat.com> Mime-Version: 1.0 References: <20231027182217.3615211-1-seanjc@google.com> <20231027182217.3615211-9-seanjc@google.com> <211d093f-4023-4a39-a23f-6d8543512675@redhat.com> Message-ID: Subject: Re: [PATCH v13 08/35] KVM: Introduce KVM_SET_USER_MEMORY_REGION2 From: Sean Christopherson To: Paolo Bonzini Cc: Marc Zyngier , Oliver Upton , Huacai Chen , Michael Ellerman , Anup Patel , Paul Walmsley , Palmer Dabbelt , Albert Ou , Alexander Viro , Christian Brauner , "Matthew Wilcox (Oracle)" , Andrew Morton , kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev, linux-mips@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Xiaoyao Li , Xu Yilun , Chao Peng , Fuad Tabba , Jarkko Sakkinen , Anish Moorthy , David Matlack , Yu Zhang , Isaku Yamahata , "=?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?=" , Vlastimil Babka , Vishal Annapurve , Ackerley Tng , Maciej Szmigiero , David Hildenbrand , Quentin Perret , Michael Roth , Wang , Liam Merwick , Isaku Yamahata , "Kirill A . Shutemov" Content-Type: text/plain; charset="us-ascii" X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 8A061180006 X-Stat-Signature: ocsqyeuj74rdzr3fcohd1qrzx8c6htrs X-Rspam-User: X-HE-Tag: 1698697553-853280 X-HE-Meta: U2FsdGVkX18XCet03Xqx4H/mM3w7pzKIIjj6KUcCw49yh+J8ckp+jJKeI9eadi5gCd/fH9SSPVmiBaJjxV968od2U9rBL1CMI4rkqcsmf8xa5Y7BecpST7a31kQLmOAw22FZLHbdtM4ewdLy1EDAvtaZvRlUCoBGrxvkm8cGJh25bGChEWdG/LKOSt8vNDKCHTe9VIsq8XWxA4J2cNQZvi2JrqgaRBvCc3StjO/WLuK8aMhhNwuIniDtze5xTMb6JBT896Gsk1FLCwfivo0odbzEF8RnbCm6pZccMo5ifELS1J+mBkU6gKnw9/siCEcdbciPjTlgWtEtUkkwwL9ILxzGHKVx2hw19d9IV66rGl6Qd1ppqjaGdUMR/I8EjchzprhH4JlnvKXXP2j+KIN5kgC/1WhSYY8e89KPAy3NOAy50IodPvGxgnIll3VxZ9NuUUHRcmRwpsoHb/duwexFYPWOlzzIokVzFrStQTsGJxP9LqkGoF1d+a9MHClQ8QvpP24eJhLj0nRIZTBuMrZj2f4rt5ky5S3wshOhyoLeWZ3H58JIeOzzQrgSnaxNp7xc5uZ6U3tArZGj79lLfeAsT6QfujuWyTsSaOIkew+7aB/tMqHzxMOjyFLD0jelg9WNE30NMpUdBXv7s61nJs620azS+NmG4UtiOuCRWySyWmnuoQ+kUqzkBU8M0wuhGZt36+fx9/buD04VRkvhX0C90+Z3Kt6Rw4sQQoJQqlYpv/Ky9SYKHXQ8cPvP9Koiye4iE08fcXL6q3OCxtOkz2NIdXm9ufLxU61Z/1G1IfuVuOnds5IUKX4mJBDAmSkMsTMcClMiNhFGuvaLQmRcSbpP2kWgmt9ygZ1YdEJP56r/CK5gxkDT3aKXNxO1MEKLHCs4zBtTQ/lff+nwNhtqRz8KhAOGlPkrqYqgIwqXTuvlt0Po5cEVYhodhDupo7z3nWbBHNcZ4z1EoiBGos/n6ih +5iPJARr ar5K/wUUIFtJIxN6+3H3ngooDXm9uKJ9apIHno60gCkK0A1/nqySo0YRdgJ2TXdJ9Z/xWBtmeseU5TW4xV+lI1LDncXgMSl6A0rkDxwCMqvVbQqV0ga4UEsrz4EJfrsMi0/9nUyjIPAXWOePxAmcuyjOqK6DXPQYdnu2pKCDeUW0XTuZ810IdqjuvqkRIdkriuYI0VcHWaIQ6qBzl6QVfRNaaC/RsrrWrl8oVJy4UWYgdbtygRR2ogItXGlilU+Oy8HqmcHKn1MVOii/C4jViTenRViGaK3pbAD/nFGM2cPq4YBQxIKzzXd+uOM0yuhCshkr9TFRLpAwWZ1t/R/cDjS8DVr7wF/bGR/BW/8N1bItlOeZ4Y0coF6MJA0/62sfRH1GBSfl47Zgerqzh5MAbxY2FxerkgiOh25ABEoj9v8Gn84nnVG+PNauhKL/TqlZYyPVWDD3xmdowDiKWuFFUlFJ/b0rAMRnJX4UBu00tgIIBYvJdmg6CaRJ0HfzrTiCjwGsDdOpleTuGXzu+d8s67iBwRol1PPyKIaMzt62dLVkEs63cGPfrH1Rb18cyVLXaa1eodxMEbEO7r7++1zNKMEoI1COZ4hMX4XQDRsKDKsiESQ+GoyEKqmbZKiiZL4l/jpRYpysamUkIW6vZpK0mDsULoCTvKTbg0udsNMWcjezFEJWlOE36VNC9GVrM8q8MaEUGAis4M28OVC8= 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: List-Subscribe: List-Unsubscribe: On Mon, Oct 30, 2023, Paolo Bonzini wrote: > On 10/27/23 20:21, Sean Christopherson wrote: > > > > + if (ioctl == KVM_SET_USER_MEMORY_REGION) > > + size = sizeof(struct kvm_userspace_memory_region); > > This also needs a memset(&mem, 0, sizeof(mem)), otherwise the out-of-bounds > access of the commit message becomes a kernel stack read. Ouch. There's some irony. Might be worth doing memset(&mem, -1, sizeof(mem)) though as '0' is a valid file descriptor and a valid file offset. > Probably worth adding a check on valid flags here. Definitely needed. There's a very real bug here. But rather than duplicate flags checking or plumb @ioctl all the way to __kvm_set_memory_region(), now that we have the fancy guard(mutex) and there are no internal calls to kvm_set_memory_region(), what if we: 1. Acquire/release slots_lock in __kvm_set_memory_region() 2. Call kvm_set_memory_region() from x86 code for the internal memslots 3. Disallow *any* flags for internal memslots 4. Open code check_memory_region_flags in kvm_vm_ioctl_set_memory_region() 5. Pass @ioctl to kvm_vm_ioctl_set_memory_region() and allow KVM_MEM_PRIVATE only for KVM_SET_USER_MEMORY_REGION2 E.g. this over ~5 patches --- arch/x86/kvm/x86.c | 2 +- include/linux/kvm_host.h | 4 +-- virt/kvm/kvm_main.c | 65 +++++++++++++++++----------------------- 3 files changed, 29 insertions(+), 42 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index e3eb608b6692..dd3e2017366c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12478,7 +12478,7 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, m.guest_phys_addr = gpa; m.userspace_addr = hva; m.memory_size = size; - r = __kvm_set_memory_region(kvm, &m); + r = kvm_set_memory_region(kvm, &m); if (r < 0) return ERR_PTR_USR(r); } diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 687589ce9f63..fbb98efe8200 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1170,7 +1170,7 @@ static inline bool kvm_memslot_iter_is_valid(struct kvm_memslot_iter *iter, gfn_ * -- just change its flags * * Since flags can be changed by some of these operations, the following - * differentiation is the best we can do for __kvm_set_memory_region(): + * differentiation is the best we can do for __kvm_set_memory_region(). */ enum kvm_mr_change { KVM_MR_CREATE, @@ -1181,8 +1181,6 @@ enum kvm_mr_change { int kvm_set_memory_region(struct kvm *kvm, const struct kvm_userspace_memory_region2 *mem); -int __kvm_set_memory_region(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem); void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot); void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen); int kvm_arch_prepare_memory_region(struct kvm *kvm, diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 23633984142f..39ceee2f67f2 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1608,28 +1608,6 @@ static void kvm_replace_memslot(struct kvm *kvm, } } -static int check_memory_region_flags(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem) -{ - u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES; - - if (kvm_arch_has_private_mem(kvm)) - valid_flags |= KVM_MEM_PRIVATE; - - /* Dirty logging private memory is not currently supported. */ - if (mem->flags & KVM_MEM_PRIVATE) - valid_flags &= ~KVM_MEM_LOG_DIRTY_PAGES; - -#ifdef __KVM_HAVE_READONLY_MEM - valid_flags |= KVM_MEM_READONLY; -#endif - - if (mem->flags & ~valid_flags) - return -EINVAL; - - return 0; -} - static void kvm_swap_active_memslots(struct kvm *kvm, int as_id) { struct kvm_memslots *slots = kvm_get_inactive_memslots(kvm, as_id); @@ -2014,11 +1992,9 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id, * space. * * Discontiguous memory is allowed, mostly for framebuffers. - * - * Must be called holding kvm->slots_lock for write. */ -int __kvm_set_memory_region(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem) +static int __kvm_set_memory_region(struct kvm *kvm, + const struct kvm_userspace_memory_region2 *mem) { struct kvm_memory_slot *old, *new; struct kvm_memslots *slots; @@ -2028,9 +2004,7 @@ int __kvm_set_memory_region(struct kvm *kvm, int as_id, id; int r; - r = check_memory_region_flags(kvm, mem); - if (r) - return r; + guard(mutex)(&kvm->slots_lock); as_id = mem->slot >> 16; id = (u16)mem->slot; @@ -2139,27 +2113,42 @@ int __kvm_set_memory_region(struct kvm *kvm, kfree(new); return r; } -EXPORT_SYMBOL_GPL(__kvm_set_memory_region); int kvm_set_memory_region(struct kvm *kvm, const struct kvm_userspace_memory_region2 *mem) { - int r; + /* Flags aren't supported for KVM-internal memslots. */ + if (WARN_ON_ONCE(mem->flags)) + return -EINVAL; - mutex_lock(&kvm->slots_lock); - r = __kvm_set_memory_region(kvm, mem); - mutex_unlock(&kvm->slots_lock); - return r; + return __kvm_set_memory_region(kvm, mem); } EXPORT_SYMBOL_GPL(kvm_set_memory_region); -static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, +static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, unsigned int ioctl, struct kvm_userspace_memory_region2 *mem) { + u32 valid_flags = KVM_MEM_LOG_DIRTY_PAGES; + + if (ioctl == KVM_SET_USER_MEMORY_REGION2 && + kvm_arch_has_private_mem(kvm)) + valid_flags |= KVM_MEM_PRIVATE; + + /* Dirty logging private memory is not currently supported. */ + if (mem->flags & KVM_MEM_PRIVATE) + valid_flags &= ~KVM_MEM_LOG_DIRTY_PAGES; + +#ifdef __KVM_HAVE_READONLY_MEM + valid_flags |= KVM_MEM_READONLY; +#endif + + if (mem->flags & ~valid_flags) + return -EINVAL; + if ((u16)mem->slot >= KVM_USER_MEM_SLOTS) return -EINVAL; - return kvm_set_memory_region(kvm, mem); + return __kvm_set_memory_region(kvm, mem); } #ifndef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT @@ -5145,7 +5134,7 @@ static long kvm_vm_ioctl(struct file *filp, if (copy_from_user(&mem, argp, size)) goto out; - r = kvm_vm_ioctl_set_memory_region(kvm, &mem); + r = kvm_vm_ioctl_set_memory_region(kvm, ioctl, &mem); break; } case KVM_GET_DIRTY_LOG: { base-commit: 881375a408c0f4ea451ff14545b59216d2923881 --