From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f73.google.com (mail-pj1-f73.google.com [209.85.216.73]) (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 C98183C583E for ; Tue, 26 May 2026 16:39:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.73 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779813586; cv=none; b=HPue3VQRVaykQ0h7j3TmU1U7n0ZJZNmYjO+iGpZLj/CgDR7zW4bjU2vPtdb8AtCm3KlUMnPzAWAT9roCfvrTydY6+Tm1c24yG4SAEO0tQ3mW1BKUtq3qJl7Tj430UjPFtl4z39IRco7+eq13aPYZzXk3wKqyj5nsmZJHVS77g94= 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=NZ9fFumT; arc=none smtp.client-ip=209.85.216.73 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="NZ9fFumT" Received: by mail-pj1-f73.google.com with SMTP id 98e67ed59e1d1-3663d5e9bf4so10934566a91.1 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=lists.linux.dev; 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=NZ9fFumTdrC+8H4wGO93Ta887gsF3tNmZvk4Q80KNF6dbFTd3EcbhmuuPPTCgJTDiI 7taazXUC4+5Q/qcSmWScirPHPa90ed4Bjhxi7op9hVuQlbJ+o6eXolwAdSYz+1v7QLx6 TvvzOCgzZ5vMOYvtP7eVtXvj5viiHGqtQhoBxt7UaDMj3ButrzYcupzLQxPf/LCjsq/F or8aQ15vD7hoK0D9dLeEwhGsnFmlOYGTZH2yTqCvgyoLIffImgQ7yUvdT+dqHLuKZxFq 32JQq6h/yD4n8XQmAiqqkAgx6wrb9hT/AP3NM+LjjdBBvWK0lVkHJPNog2TowciwUOY0 Qulw== 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=QiPDf4ujHIjW2JPvh8TXazhgNLFSGCVfvyKztMokFl+eJz9goj6hgkOMD1Gmo5A3oD m3eveNmlFRuBP0HeWpxtPlAk3nv8Vjk1MT3KAtK9G7/9Q5fCK5mBG848vj79of4RGJDQ CO7dGHYkHHldQirXw/AX51PczXAdUdn7vM8W0Ltuyaphy+ch3zjYchiRwgBxbrrBcZXg MZC4HRBHbeOMH9JibYK6QRMkWyRk+oT4prUqFFxINlwUa95FvihKyce2qJpWql9JePLs 7In2w9Jyt/QNg+LBWVWcUHRxxXzPXCE1ErUro8VeX+smsj3xOyxebGEoSJbC3LBcrDpx qwiw== X-Forwarded-Encrypted: i=1; AFNElJ+cAOF1I3N5VimY42NeMeyH5xW1bwAGs5w38IdoQT8g/XMA8ZfPgVNQf3dstix/9WHvQltQiyJB9BD/@lists.linux.dev X-Gm-Message-State: AOJu0Yxu57EgLnyWw0t+26uSpB/tluYFdlhQ+oe3GNBbG1hKIrOkHbB2 swzC3Ix8BvaXfMgi6EdTJ9g9vgEnpTps3gtHd9ojJ1TUbUAGtz3QMm3hIevKfRcxV47Y/bPBjUq nzgrvoQ== 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-coco@lists.linux.dev 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;