Message ID | 20230802051700.52321-2-likexu@tencent.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f41:0:b0:3e4:2afc:c1 with SMTP id v1csp255826vqx; Tue, 1 Aug 2023 23:49:41 -0700 (PDT) X-Google-Smtp-Source: APBJJlGjHVmvsmwmogynvENudpJ9gjAbbDz3U6dbHhvZb+u+VnwHNAEwRREv/yLx4lSdC463nzy5 X-Received: by 2002:a17:902:d4c8:b0:1bb:b34b:73a with SMTP id o8-20020a170902d4c800b001bbb34b073amr18104258plg.25.1690958981559; Tue, 01 Aug 2023 23:49:41 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1690958981; cv=none; d=google.com; s=arc-20160816; b=fCJWDmNX8VNhVqj8mdJcV1kBnL3A66lexto2HtQQky+62y8wCVM+6+djvef60YXhnC qGv6qer5x/4YZQoTE+ZfxnYs5RE5fOMRkwpfCsY41vP6WC/kZ9kAuOM3CWu46xL8qKQQ Z1jY0iSjYYt04PwQHyj+R9FEwpJ/sl4I/Akehf8D5ftvYeE0loprZJX76bcYO+HhbanI 0I3Q9AkwJKxsx+aRL8310nEGtEsLUV4LKJsVUVOu7dGmyjVb35/yoiRWFwRsKRQVdVs8 XWGW6oZbzZhJKJQ4XWE9Svt+DNa2gHOgLtNdJS0eNkmeqWz16LsAi/Sm/RhuEz+dukbf SqFA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=5aA5JIT6ardwan6m5SFrBRELKgm9+/graZUkPPo07yI=; fh=dCmVJW+mP4WblL3w0/L59D/usNysupsq+BS1oyQ2aNA=; b=xcoMeV1tTjHiGtlHVtXjNJxrqJjsRih0xT6Ju50bJLF240ZMmjqY6uTld1vKu8IEtQ GYoGrK2fQLqsp3XISQST9Rl/XR6l/um7eaInte9cokZ33fhCBd6c/7ae6n+bUGzM1LUN 59V0qVYOCOAp2g5oMmcBH0ymn1xYFNSx8y7E3FazMF5xziQR+/kr2eJ7MtofPGGltwkW TI+x6RvY6XGwFg0bonFgXrVdgjbhnLyfP5xS7SqVtbz6yfjNkslAI5D6ghzkF0En6SQe J2FTGMCAyYuRAn/YfQ+qCaDAOnCq7fl+YCnpwkfmMlpJDwmsH8fsI1iGtbpEMlM+EaX5 XMnA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=q7uCK5Vo; 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 f11-20020a170902ce8b00b001bbf89feae3si7180246plg.600.2023.08.01.23.49.27; Tue, 01 Aug 2023 23:49:41 -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=q7uCK5Vo; 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 S232369AbjHBFR3 (ORCPT <rfc822;maxi.paulin@gmail.com> + 99 others); Wed, 2 Aug 2023 01:17:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41596 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231378AbjHBFRY (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 2 Aug 2023 01:17:24 -0400 Received: from mail-pl1-x630.google.com (mail-pl1-x630.google.com [IPv6:2607:f8b0:4864:20::630]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7013F1BFD; Tue, 1 Aug 2023 22:17:22 -0700 (PDT) Received: by mail-pl1-x630.google.com with SMTP id d9443c01a7336-1b9c5e07c1bso54640345ad.2; Tue, 01 Aug 2023 22:17:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1690953442; x=1691558242; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=5aA5JIT6ardwan6m5SFrBRELKgm9+/graZUkPPo07yI=; b=q7uCK5VordhnMcT6Tz24S/dN/Vn2wrSzLgX2dpILJ6wRV6TK6oN2TyUS6RqCEyV7hL nntU07Yf24yserlGDfyafGLY9SzFAEsKDmfQHHEDv9RqIeqOkmAer9sRMOcONoOdzqpp nOlHhvW5PXBQb62KXzZZZx+QL7xwtHw+88lyK+MRRl70SEKwGGihs/H7LU5KyZimLcuQ G/Lny3nf6gW0DdDIprZEFHbuwDJWIsA/KCtAZnw5+nIe+YXV/PWWRSqqwukDSB9VB+Sq BvQ4VI/gyuSHWg/15j1wrojTrs+hE+kIUS9Adqj1vazvrRYP70DsrTMNUfQYv1Y99+TT +wgw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690953442; x=1691558242; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5aA5JIT6ardwan6m5SFrBRELKgm9+/graZUkPPo07yI=; b=MDCbwh42FETez1Fh3+KR3Y7CoU4NBDzbFuW0+qDMRjHKzvkbw13bAeY9cHItkowCrr eBCTsIGjgHAXoG3gPE9us7gZl+LhWukPdf7xUCHy7Nmt7l2QL+zlYgOxtXUKmrsLldSR f/ZQcQVlPBHQyF2QEEdPP963YQiI/O/1VJ4AOaQpJjB5SHy01bOCV8DbEmpA8IgLYWrW YW3QCfzHzobpn7QycSUm91fsMEqLJEbxZPGJ2oVBpbu34Eapd4s+cBhLFOYQ1Qt+vpml kjcWrfSpGb1m4GYAj9Rlg/8avfwXMIRgvjoYofj2jd9huOjYAHovRzThBCLt0D9TPHxl AOwg== X-Gm-Message-State: ABy/qLYFR4LWHz0ZTbYdIharnItlk3PnhK9czg5xJA3He8sGdSD6+0RO 1wzfVqICUNBUVMr8fZCUlK4= X-Received: by 2002:a17:903:245:b0:1b8:abe7:5a80 with SMTP id j5-20020a170903024500b001b8abe75a80mr17356754plh.40.1690953441838; Tue, 01 Aug 2023 22:17:21 -0700 (PDT) Received: from localhost.localdomain ([103.7.29.32]) by smtp.gmail.com with ESMTPSA id f8-20020a17090274c800b001bba7002132sm11330446plt.33.2023.08.01.22.17.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Aug 2023 22:17:21 -0700 (PDT) From: Like Xu <like.xu.linux@gmail.com> X-Google-Original-From: Like Xu <likexu@tencent.com> To: Alex Williamson <alex.williamson@redhat.com>, Paolo Bonzini <pbonzini@redhat.com> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: [PATCH v2 1/2] KVM: eventfd: Fix NULL deref irqbypass producer Date: Wed, 2 Aug 2023 13:16:59 +0800 Message-ID: <20230802051700.52321-2-likexu@tencent.com> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20230802051700.52321-1-likexu@tencent.com> References: <20230802051700.52321-1-likexu@tencent.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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, 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: 1773099004839125912 X-GMAIL-MSGID: 1773099004839125912 |
Series |
KVM: irqbypass: XArray conversion and a deref fix
|
|
Commit Message
Like Xu
Aug. 2, 2023, 5:16 a.m. UTC
From: Like Xu <likexu@tencent.com> Adding guard logic to make irq_bypass_register/unregister_producer() looks for the producer entry based on producer pointer itself instead of pure token matching. As was attempted commit 4f3dbdf47e15 ("KVM: eventfd: fix NULL deref irqbypass consumer"), two different producers may occasionally have two identical eventfd's. In this case, the later producer may unregister the previous one after the registration fails (since they share the same token), then NULL deref incurres in the path of deleting producer from the producers list. Registration should also fail if a registered producer changes its token and registers again via the same producer pointer. Cc: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Like Xu <likexu@tencent.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> --- virt/lib/irqbypass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Comments
Hi Alex, what do you think of Sean's proposal diff below ? By the way, could anyone to accept patch 1/2, thanks. On 17/8/2023 2:37 am, Sean Christopherson wrote: > On Wed, Aug 02, 2023, Like Xu wrote: >> From: Like Xu <likexu@tencent.com> >> >> Adding guard logic to make irq_bypass_register/unregister_producer() >> looks for the producer entry based on producer pointer itself instead >> of pure token matching. >> >> As was attempted commit 4f3dbdf47e15 ("KVM: eventfd: fix NULL deref >> irqbypass consumer"), two different producers may occasionally have two >> identical eventfd's. In this case, the later producer may unregister >> the previous one after the registration fails (since they share the same >> token), then NULL deref incurres in the path of deleting producer from >> the producers list. >> >> Registration should also fail if a registered producer changes its >> token and registers again via the same producer pointer. >> >> Cc: Alex Williamson <alex.williamson@redhat.com> >> Signed-off-by: Like Xu <likexu@tencent.com> >> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> >> --- >> virt/lib/irqbypass.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c >> index 28fda42e471b..e0aabbbf27ec 100644 >> --- a/virt/lib/irqbypass.c >> +++ b/virt/lib/irqbypass.c >> @@ -98,7 +98,7 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer) >> mutex_lock(&lock); >> >> list_for_each_entry(tmp, &producers, node) { >> - if (tmp->token == producer->token) { >> + if (tmp->token == producer->token || tmp == producer) { >> ret = -EBUSY; >> goto out_err; >> } >> @@ -148,7 +148,7 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer) >> mutex_lock(&lock); >> >> list_for_each_entry(tmp, &producers, node) { >> - if (tmp->token != producer->token) >> + if (tmp != producer) > > What are the rules for using these APIs? E.g. is doing unregister without > first doing a register actually allowed? Ditto for having multiple in-flight > calls to (un)register the exact same producer or consumer. > > E.g. can we do something like the below, and then remove the list iteration to > find the passed in pointer (which is super odd IMO). Obviously not a blocker > for this patch, but it seems like we could achieve a simpler and more performant > implementation if we first sanitize the rules and the usage. > > diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c > index 28fda42e471b..be0ba4224a23 100644 > --- a/virt/lib/irqbypass.c > +++ b/virt/lib/irqbypass.c > @@ -90,6 +90,9 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer) > if (!producer->token) > return -EINVAL; > > + if (WARN_ON_ONCE(producer->node.prev && !list_empty(&producer->node))) > + return -EINVAL; > + > might_sleep(); > > if (!try_module_get(THIS_MODULE)) > @@ -140,6 +143,9 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer) > if (!producer->token) > return; > > + if (WARN_ON_ONCE(!producer->node.prev || list_empty(&producer->node))) > + return; > + > might_sleep(); > > if (!try_module_get(THIS_MODULE)) >
diff --git a/virt/lib/irqbypass.c b/virt/lib/irqbypass.c index 28fda42e471b..e0aabbbf27ec 100644 --- a/virt/lib/irqbypass.c +++ b/virt/lib/irqbypass.c @@ -98,7 +98,7 @@ int irq_bypass_register_producer(struct irq_bypass_producer *producer) mutex_lock(&lock); list_for_each_entry(tmp, &producers, node) { - if (tmp->token == producer->token) { + if (tmp->token == producer->token || tmp == producer) { ret = -EBUSY; goto out_err; } @@ -148,7 +148,7 @@ void irq_bypass_unregister_producer(struct irq_bypass_producer *producer) mutex_lock(&lock); list_for_each_entry(tmp, &producers, node) { - if (tmp->token != producer->token) + if (tmp != producer) continue; list_for_each_entry(consumer, &consumers, node) {