Message ID | ZKyEL/4pFicxMQvg@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a6b2:0:b0:3e4:2afc:c1 with SMTP id c18csp111947vqm; Mon, 10 Jul 2023 15:34:29 -0700 (PDT) X-Google-Smtp-Source: APBJJlGANFFrM9IO4+YjMfCf+jn56UtXNvtwlLKci6dRQTM8j8MPO3MDw9hdz3RFszfUsjR7flAt X-Received: by 2002:a17:903:2449:b0:1b8:9b1d:d7f1 with SMTP id l9-20020a170903244900b001b89b1dd7f1mr12954972pls.47.1689028469606; Mon, 10 Jul 2023 15:34:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689028469; cv=none; d=google.com; s=arc-20160816; b=uujxrVuZ3nAKiKmmd//q6awTNwemq9CD/4B1/y+PWDkdJc4SYKopO7JVdjED7w+tg7 DCbm3+qDSekika9FamLj7NL0xf5IMIt3nu7gngXd15umY9nSvB2Yu4kXIeDcgn83nNG5 F8JzrYs7v1V1oqXjZWLoJVvUvgDE4IJF02QgiDDK7XsKrVtAn/8NHmlGaRzuYiiAHX3B 5tBpnelDKc+fVQNHs6AeYYmyqoVniIfOYdkhnC7zFppCF4hkl22VRR40Joiih6GRhkrM eJqhmY2Gd4v1FpPIkmWyqGGHUW5Cy/XGDWNfPN9xyrXQm+hknlbV4jN8S/k/rn7V9aJ0 H+nw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=H18CdgrYMTpofy+sXOjkYOSJshvEuHvdbWqmwdvSi4E=; fh=qN83ZSlvGTeEz3Vahn1kKYHGpEYgB0rGii+dzANbOFo=; b=mJ7BUvzehSEYIE5jQgcXC9GF11JF6ySoq5IUwp8h8SGnd6XAAFm3wjO1ggjKc5AqZH MeKkaCvam4tDVGgvA8S+kxtufd6mAOTYWqKFibbmZ2WVqbU6XT/Ex3kEZEopZYRAbrMm zCWRhgRmz6GubZZ6yg+soFwLDaFsBoE9Fn0c12q5lSXPQFVtmojUZekDkSu77s5CNg5M SYRdhojpLmbQf26c7u+eviA8fkWftKNY0A4W3PuZtFfCjswPM8Z5OTelgj5HyREeQwKi oXIs2CnzeQ0/Exuk9NAASpodb8keXSjwUVpoF5eXaIfBLdUQFBXVk1Lg61B/YauMQuxp bB1A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=cUtay2Md; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b6-20020a170902d50600b001b3d6d125f9si464685plg.524.2023.07.10.15.34.16; Mon, 10 Jul 2023 15:34:29 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=cUtay2Md; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230262AbjGJWUh (ORCPT <rfc822;gnulinuxfreebsd@gmail.com> + 99 others); Mon, 10 Jul 2023 18:20:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54546 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229659AbjGJWUg (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 10 Jul 2023 18:20:36 -0400 Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9A568189; Mon, 10 Jul 2023 15:20:35 -0700 (PDT) Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1b9cdef8619so13471445ad.0; Mon, 10 Jul 2023 15:20:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689027635; x=1691619635; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :from:to:cc:subject:date:message-id:reply-to; bh=H18CdgrYMTpofy+sXOjkYOSJshvEuHvdbWqmwdvSi4E=; b=cUtay2Mdoywod53JzsZMnBIYUUqlsr/H+h4hz7hE+usjY4rOuuGXMRk/bI/Wwt9Bke qZTt1llHZDRCmn9y5ohESwrecyx8J4bzAIKEb2rWRCp2XDuOwz7+Z2wl2QlHW9x0F05Q BlPGXe3mrcoHmdlXWKHuBoj3h8kZvLk4JKLv0GwsMM/ciDNm+SUDltWzskRVUnq2v6gU nwdvBsKVxbIX7/9msIbibQLglMxO/iQVYy5AKPkEX0/PY0iPB+gIsLjRGDmCo5b58QOF E/I72QXheGOaSJbYJIJuLHvHIHSOXmqEGzELpzwXlC2PqUMYKe4JVT8Rp8ceXFtB67fa yB+g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689027635; x=1691619635; h=content-disposition:mime-version:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=H18CdgrYMTpofy+sXOjkYOSJshvEuHvdbWqmwdvSi4E=; b=TYIM4l6oyrhFOJyqHrFQVLxXFNlb+Apdc0kYXulmtPB5zwztj6LJmVp8CDx0+3/Sey q0Ck0aOL/eHp4mDdSHSWlv2Rh1HEkMTM9XY0tpyMoHuo1MeGUS9Rj9ru3TJTX701AGI7 518i+M+47ayRTpRWNyiys3TUJEuR5MRbXQU6f0ONuGnOboWzhu+2XIvD+L8pabwaqXsa xP8tqkcddmvle8dx5sbwLCzgSs/D7cEGH7zO0CjhHtkAlB771v1xwsNqIGAeZCzBy8wY t/Nspg7plBAPe+0SU+fYdtngf1mIZTROG7gjZmWOfL097g3eoOXruQJ77YvFj/1TEWGW 9CUQ== X-Gm-Message-State: ABy/qLYfM4+GMWfmpADmw6nr7j/hSbzGUlQuNAzcX5Bf2ic7uEVXYHUi HVP82ndSEL82rND/IJ+yF5BF73aU4Vo= X-Received: by 2002:a17:902:f546:b0:1b0:3df7:5992 with SMTP id h6-20020a170902f54600b001b03df75992mr14716372plf.32.1689027634962; Mon, 10 Jul 2023 15:20:34 -0700 (PDT) Received: from google.com ([2620:15c:9d:2:96bf:7e77:39eb:7a23]) by smtp.gmail.com with ESMTPSA id bb10-20020a170902bc8a00b001b9c5e07bc3sm364766plb.238.2023.07.10.15.20.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 10 Jul 2023 15:20:34 -0700 (PDT) Date: Mon, 10 Jul 2023 15:20:31 -0700 From: Dmitry Torokhov <dmitry.torokhov@gmail.com> To: Paolo Bonzini <pbonzini@redhat.com> Cc: Greg KH <gregkh@linuxfoundation.org>, Sean Christopherson <seanjc@google.com>, Roxana Bradescu <roxabee@google.com>, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] kvm/vfio: ensure kvg instance stays around in kvm_vfio_group_add() Message-ID: <ZKyEL/4pFicxMQvg@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,FSL_HELO_FAKE, RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771074716329478399 X-GMAIL-MSGID: 1771074716329478399 |
Series |
kvm/vfio: ensure kvg instance stays around in kvm_vfio_group_add()
|
|
Commit Message
Dmitry Torokhov
July 10, 2023, 10:20 p.m. UTC
kvm_vfio_group_add() creates kvg instance, links it to kv->group_list,
and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after
dropping kv->lock. If we race group addition and deletion calls, kvg
instance may get freed by the time we get around to calling
kvm_vfio_file_set_kvm().
Fix this by moving call to kvm_vfio_file_set_kvm() under the protection
of kv->lock. We already call it while holding the same lock when vfio
group is being deleted, so it should be safe here as well.
Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
virt/kvm/vfio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On Mon, 10 Jul 2023 15:20:31 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > dropping kv->lock. If we race group addition and deletion calls, kvg > instance may get freed by the time we get around to calling > kvm_vfio_file_set_kvm(). > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > of kv->lock. We already call it while holding the same lock when vfio > group is being deleted, so it should be safe here as well. > > Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()") > Cc: stable@vger.kernel.org > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > virt/kvm/vfio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 9584eb57e0ed..cd46d7ef98d6 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) > list_add_tail(&kvg->node, &kv->group_list); > > kvm_arch_start_assignment(dev->kvm); > + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > mutex_unlock(&kv->lock); > > - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > kvm_vfio_update_coherency(dev); > > return 0; I'm not sure this hasn't been an issue since it was originally introduced in 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when group add/delete"). The change added by the blamed ba70a89f3c2a in this respect is simply that we get the file pointer from the mutex protected object, but that mutex protected object is also what maintains that the file pointer is valid. The vfio_group implementation suffered the same issue, the delete path could put the group reference, which could theoretically cause a use after free of the vfio_group. We could effectively restore the pre-ba70a89f3c2a behavior by replacing kvg->file with filp here, but that would still leave us vulnerable to the original issue. Note also that kvm_vfio_update_coherency() takes the same mutex separately, I wonder if it wouldn't make more sense if it were moved under the caller's mutex to avoid bouncing the lock and unnecessarily taking it in the release path. Thanks, Alex
Hi Alex, On Thu, Jul 13, 2023 at 12:48:11PM -0600, Alex Williamson wrote: > On Mon, 10 Jul 2023 15:20:31 -0700 > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > > dropping kv->lock. If we race group addition and deletion calls, kvg > > instance may get freed by the time we get around to calling > > kvm_vfio_file_set_kvm(). > > > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > > of kv->lock. We already call it while holding the same lock when vfio > > group is being deleted, so it should be safe here as well. > > > > Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()") > > Cc: stable@vger.kernel.org > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > virt/kvm/vfio.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > > index 9584eb57e0ed..cd46d7ef98d6 100644 > > --- a/virt/kvm/vfio.c > > +++ b/virt/kvm/vfio.c > > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) > > list_add_tail(&kvg->node, &kv->group_list); > > > > kvm_arch_start_assignment(dev->kvm); > > + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > > > mutex_unlock(&kv->lock); > > > > - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > kvm_vfio_update_coherency(dev); > > > > return 0; > > > I'm not sure this hasn't been an issue since it was originally > introduced in 2fc1bec15883 ("kvm: set/clear kvm to/from vfio_group when > group add/delete"). > > The change added by the blamed ba70a89f3c2a in this respect is simply > that we get the file pointer from the mutex protected object, but that > mutex protected object is also what maintains that the file pointer is > valid. The vfio_group implementation suffered the same issue, the > delete path could put the group reference, which could theoretically > cause a use after free of the vfio_group. Yes, you are right, I'll update the patch with the correct "Fixes". > > We could effectively restore the pre-ba70a89f3c2a behavior by replacing > kvg->file with filp here, but that would still leave us vulnerable to > the original issue. > > Note also that kvm_vfio_update_coherency() takes the same mutex > separately, I wonder if it wouldn't make more sense if it were moved > under the caller's mutex to avoid bouncing the lock and unnecessarily > taking it in the release path. Thanks, I think I will make it a separate patch. Thanks.
On Mon, Jul 10, 2023 at 03:20:31PM -0700, Dmitry Torokhov wrote: > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > dropping kv->lock. If we race group addition and deletion calls, kvg > instance may get freed by the time we get around to calling > kvm_vfio_file_set_kvm(). > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > of kv->lock. We already call it while holding the same lock when vfio > group is being deleted, so it should be safe here as well. > > Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()") > Cc: stable@vger.kernel.org > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > virt/kvm/vfio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This looks correct, I don't know of any lock cylces that could form with kv->lock at least Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Mon, Jul 10, 2023 at 03:20:31PM -0700, Dmitry Torokhov wrote: > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > dropping kv->lock. If we race group addition and deletion calls, kvg > instance may get freed by the time we get around to calling > kvm_vfio_file_set_kvm(). > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > of kv->lock. We already call it while holding the same lock when vfio > group is being deleted, so it should be safe here as well. > > Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()") > Cc: stable@vger.kernel.org > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > virt/kvm/vfio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > index 9584eb57e0ed..cd46d7ef98d6 100644 > --- a/virt/kvm/vfio.c > +++ b/virt/kvm/vfio.c > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) > list_add_tail(&kvg->node, &kv->group_list); > > kvm_arch_start_assignment(dev->kvm); > + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > mutex_unlock(&kv->lock); > > - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > kvm_vfio_update_coherency(dev); > > return 0; > -- > 2.41.0.255.g8b1d071c50-goog What ever happened to this change? Did it end up in a KVM tree somewhere? thanks, greg k-h
On Mon, Jul 31, 2023 at 02:02:59PM +0200, Greg KH wrote: > On Mon, Jul 10, 2023 at 03:20:31PM -0700, Dmitry Torokhov wrote: > > kvm_vfio_group_add() creates kvg instance, links it to kv->group_list, > > and calls kvm_vfio_file_set_kvm() with kvg->file as an argument after > > dropping kv->lock. If we race group addition and deletion calls, kvg > > instance may get freed by the time we get around to calling > > kvm_vfio_file_set_kvm(). > > > > Fix this by moving call to kvm_vfio_file_set_kvm() under the protection > > of kv->lock. We already call it while holding the same lock when vfio > > group is being deleted, so it should be safe here as well. > > > > Fixes: ba70a89f3c2a ("vfio: Change vfio_group_set_kvm() to vfio_file_set_kvm()") > > Cc: stable@vger.kernel.org > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > virt/kvm/vfio.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c > > index 9584eb57e0ed..cd46d7ef98d6 100644 > > --- a/virt/kvm/vfio.c > > +++ b/virt/kvm/vfio.c > > @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) > > list_add_tail(&kvg->node, &kv->group_list); > > > > kvm_arch_start_assignment(dev->kvm); > > + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > > > mutex_unlock(&kv->lock); > > > > - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); > > kvm_vfio_update_coherency(dev); > > > > return 0; > > -- > > 2.41.0.255.g8b1d071c50-goog > > What ever happened to this change? Did it end up in a KVM tree > somewhere? It was posted as: https://lore.kernel.org/all/20230714224538.404793-1-dmitry.torokhov@gmail.com/ and I believe Alex Williamson is planning to take it through VFIO tree. Thanks.
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index 9584eb57e0ed..cd46d7ef98d6 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -179,10 +179,10 @@ static int kvm_vfio_group_add(struct kvm_device *dev, unsigned int fd) list_add_tail(&kvg->node, &kv->group_list); kvm_arch_start_assignment(dev->kvm); + kvm_vfio_file_set_kvm(kvg->file, dev->kvm); mutex_unlock(&kv->lock); - kvm_vfio_file_set_kvm(kvg->file, dev->kvm); kvm_vfio_update_coherency(dev); return 0;