From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f74.google.com (mail-pj1-f74.google.com [209.85.216.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 851E13CF696 for ; Tue, 26 May 2026 16:39:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.74 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779813586; cv=none; b=rUaPPZ6xUoEA8Q9BQ1boViXBKUEorTO37avsxN8t1x2dtJZ9sBq875LhNy4HZKSGb/mROK5au0sDcR4aREB8vVM5A0a+ptRS3IYu1lq5hD4xiZyZqgp4yh31Fha/v/Uz3OdeXEvLvT6rjsbzUh8gS+hGcj5ySQkVUySuI8pFX1w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779813586; c=relaxed/simple; bh=vdiS8jEUTDZVhvBUoPc9BRMQcliTq3ZdcyymjfOkB6E=; h=Date:In-Reply-To:Mime-Version:References:Message-ID:Subject:From: To:Cc:Content-Type; b=bmFREM+5yqPI8ui+Kwqe5jNEc69xQN5ZtLNGTNkz6d6OeImtW7ZfhOG6yEMJYgsVjyApJ8g9X5cAkByBd0j6Gl5hTr+BwefaZ9ShlSsFEHwPHMuKUu/AGXsgk2aoTwUQ7ZvZBNMQ5LaMLSlRaFyqWjQBe2A+74eTbFYTtFyngZM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=MWuQ1+gn; arc=none smtp.client-ip=209.85.216.74 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=flex--seanjc.bounces.google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="MWuQ1+gn" Received: by mail-pj1-f74.google.com with SMTP id 98e67ed59e1d1-367fd7b8825so10774402a91.0 for ; Tue, 26 May 2026 09:39:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1779813581; x=1780418381; darn=vger.kernel.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=gGCac3ndRY4cFGyojmw/sDEy33dzfMnUmsB2axatg3k=; b=MWuQ1+gnVcwNuZkWrR0B5BZn4qfgG8DGjiWBMIpeiHDCr4VHX+nhCBHmy2xi1XKs7S lC/dgns3QKEE0p7XY4erG1qXKomMzwd7wTdmlPCUCDhqM6OY1OFYIFGTIRinsQPZwtrb v4sPS08swQdX8a4G2qyZxyCV4DV25CJIAINLhl6ltO/9+QWozo8FQAO2RHMdvka0ch1S x3/2gM3H2NbYe6+/a3Rp35djW48ybxrOAGqFoFgJkxIcOhrPtO7LUFvl0bb0MCpT55dN 2eU+x0l2FIrdAZr0f8T5qKsz3zhp6cTnsXSeLK1DDYn1BNUYrZJ4Htgq05YCXBpCP45V URnQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779813581; x=1780418381; 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=gGCac3ndRY4cFGyojmw/sDEy33dzfMnUmsB2axatg3k=; b=S3MSstcOrZnhQWav1O3a3F80Iuut1EZ6f0MgkwPEDoNhNhqyttkzBmjwtYPuXhHJtd KP9M+FFCIw7zNRXRfEKYeteY6DbbaOcWYt5GFzYPjhH3uz7YUfrCaxOMYMGAV01JMhK0 iaw5VsRU66xgvm3Wp5qYd9I2FsGmGIgPWCpoj/UauEtj1cC2GJndPaDmjjC4+VSUmzoY SJa3yge5UK8IruKoBbWs2vqt2nRYjTRaUecSx5vnsaV1F6VEjpkqr+ZCjqLnoFIzBJ7t lWV7/tnqcaPUczRWnMSklOjgsuYMBC9fO3jfnjt7RjvzYq0z7yY2USYQpfXreXJ8uHI2 L2Vw== X-Forwarded-Encrypted: i=1; AFNElJ9AyHyCmKeW+BBqQVnIxl/8H3sYHzLyCFE7wkm9uNWFoDP+HHxy0DIFPYVw+4GqyyZE+p0zfGYI42I8f4M=@vger.kernel.org X-Gm-Message-State: AOJu0Yx0a0PoPo6Hb5rt3mLhgeyqUbxuqBfS0yYVBwhDNegM+oeE5Xjv R7OnSQB4UfZ60ubcRxKvkFafnDx4o28rR7CoGQN05ilBkjsb35utfQgseSaJvqgcqMIwcxewnbY L+dadyA== X-Received: from pghq18.prod.google.com ([2002:a63:e212:0:b0:c6e:6f94:b489]) (user=seanjc job=prod-delivery.src-stubby-dispatcher) by 2002:a17:90b:270e:b0:362:e826:cefe with SMTP id 98e67ed59e1d1-36a67697a77mr19184155a91.23.1779813580821; Tue, 26 May 2026 09:39:40 -0700 (PDT) Date: Tue, 26 May 2026 09:39:40 -0700 In-Reply-To: <20260522-fix-sev-gmem-post-populate-v2-3-3f196bfad5a1@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 References: <20260522-fix-sev-gmem-post-populate-v2-0-3f196bfad5a1@google.com> <20260522-fix-sev-gmem-post-populate-v2-3-3f196bfad5a1@google.com> Message-ID: Subject: Re: [PATCH v2 3/5] KVM: guest_memfd: Handle errors from xa_store_range() when binding From: Sean Christopherson To: Ackerley Tng Cc: Paolo Bonzini , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , Kiryl Shutsemau , Rick Edgecombe , Vishal Annapurve , Yan Zhao , Michael Roth , Isaku Yamahata , Chao Peng , Xiaoyao Li , Zongyao Chen , kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-coco@lists.linux.dev, Yu Zhang , Fuad Tabba Content-Type: text/plain; charset="us-ascii" On Fri, May 22, 2026, Ackerley Tng wrote: > Unhandled errors from xa_store_range() means kvm_gmem_bind() might falsely > reporting success, leading to false assumptions in guest_memfd's lifecycle > later. > > On error, restore the unbound state and return the error to userspace. > > Fixes: a7800aa80ea4d ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory") > Signed-off-by: Ackerley Tng > --- > virt/kvm/guest_memfd.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c > index d203135969d13..5b4911ffa208a 100644 > --- a/virt/kvm/guest_memfd.c > +++ b/virt/kvm/guest_memfd.c > @@ -648,6 +648,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, > struct inode *inode; > struct file *file; > int r = -EINVAL; > + void *result; I would rather go with "xr". "result" is too generic, e.g. begs the question of "result of what?" Actually, I don't think we even need an intermediate variable. > BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff)); > > @@ -688,7 +689,14 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, > if (kvm_gmem_supports_mmap(inode)) > slot->flags |= KVM_MEMSLOT_GMEM_ONLY; > > - xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL); > + result = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL); > + if (xa_is_err(result)) { > + r = xa_err(result); > + xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL); I'm not convinced this is necessary. Sashiko "asked" the question: : If xa_store_range() fails midway through storing a large range (for example, : returning -ENOMEM), does it leave the already-processed entries in the : f->bindings XArray? : : When this error is propagated back, the caller __kvm_set_memory_region() : will abort the operation and free the memslot without calling : kvm_gmem_unbind(). : : Since the partial XArray updates aren't rolled back here, could this leave : dangling pointers to the freed memslot in f->bindings? If so, when the file : is eventually closed, kvm_gmem_release() might iterate over these dangling : pointers and write to slot->gmem.file, resulting in a use-after-free. but I think Sashiko is hallicunating. If @entry is non-NULL, xa_store_range() pre-creates the entire range, before storing anything into the range: if (entry) { unsigned int order = BITS_PER_LONG; if (last + 1) order = __ffs(last + 1); xas_set_order(&xas, last, order); xas_create(&xas, true); if (xas_error(&xas)) goto unlock; } Yes, the API handles failure on the subsequent xas_store(), but I can't imagine that failure is actually, barring garbage input from KVM: do { xas_set_range(&xas, first, last); xas_store(&xas, entry); if (xas_error(&xas)) goto unlock; first += xas_size(&xas); } while (first <= last); Purely from a design perspective, providing an API that can fail partway through under normal operation, with no indication of where failure occured (AFAICT), would be awful. > + } else { > + r = 0; > + } > + > filemap_invalidate_unlock(inode->i_mapping); > > /* > @@ -696,7 +704,6 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, > * not the other way 'round. Active bindings are invalidated if the > * file is closed before memslots are destroyed. > */ > - r = 0; All in all, unless someone proves with a test that I'm wrong, just this? diff --git virt/kvm/guest_memfd.c virt/kvm/guest_memfd.c index 0c923fd603fd..c0f5b9565be2 100644 --- virt/kvm/guest_memfd.c +++ virt/kvm/guest_memfd.c @@ -688,7 +688,7 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, if (kvm_gmem_supports_mmap(inode)) slot->flags |= KVM_MEMSLOT_GMEM_ONLY; - xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL); + r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL)); filemap_invalidate_unlock(inode->i_mapping); /* @@ -696,7 +696,6 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot, * not the other way 'round. Active bindings are invalidated if the * file is closed before memslots are destroyed. */ - r = 0; err: fput(file); return r;