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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0DD63C41513 for ; Wed, 16 Aug 2023 18:37:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345585AbjHPSh1 (ORCPT ); Wed, 16 Aug 2023 14:37:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36036 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345606AbjHPShT (ORCPT ); Wed, 16 Aug 2023 14:37:19 -0400 Received: from mail-pg1-x54a.google.com (mail-pg1-x54a.google.com [IPv6:2607:f8b0:4864:20::54a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0E0081986 for ; Wed, 16 Aug 2023 11:37:18 -0700 (PDT) Received: by mail-pg1-x54a.google.com with SMTP id 41be03b00d2f7-55c79a5565aso7427588a12.3 for ; Wed, 16 Aug 2023 11:37:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1692211037; x=1692815837; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=R7fdKmEY7bD6zEhbTIZAzgi0Gq+GtDgf7Fev/C7AAGM=; b=rLZhtE01pc4I/k71SWB8oJkUqzVgoZkl9ke1X5JLEjFj6IuHfSYzT0dm1pZJrfdYDF 3z2/TcRKzbCXR4Op2KduCzCGnYNexhlMbsIUtEHK3vCqhPslckbETavmtCH8tgH6WfM8 XwLxhZGD4wqa370lTsFYHEL3kMYp+Q+wgfFVEkNNGwWFA5ccGjBywHW9K1uzXkHDsl/Z 3TezHRxWqpSQRmzvd3I6D0IblynqsTWLj8uUhvUTQAMLghf0S9VIuxazRs91Ov1BpHve Z33yEeSf1kIy19Iu92oO0x0tPBZ5bvQpdHSYnafd9DI8M2bQr2lV36BWBEGofOG21pN8 nkbw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1692211037; x=1692815837; 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=R7fdKmEY7bD6zEhbTIZAzgi0Gq+GtDgf7Fev/C7AAGM=; b=C1LO9hUzYwbpzxbdw7hA4+WrUPhmCVk5FSC31HidamtvlEhNEoGM43RBk9wXJxEyRN yS+yv+jGeMikABJZbgaaWB8zjYQYwCKaMz9ZphJwas65oinlusB6YkSaJhAYmFUJYqr6 qhd+OtlCc2j7RFOfJ6Buky1COO67y83tuvrTiB7/wfW03whYkoXfHxWVNYSc+e1RZe3/ ZqanOJP01LVwk60BBcm7gJ5WZ/SDFuF1I5Yhe1arRD5Mgzso8WYDnAx2zwzfPdDClAED SRzf1qSTjdBh1BuvURga+FO9nMgoSo9iefIJ7wz9TBV6r2ItMRCf2a0faEUX3n6xeEaM ka1A== X-Gm-Message-State: AOJu0YwTkIG5YOdFzeCmoBxU3iuEa7ExIlvsjRae8Nc1rrcIpPme3XNr ItKMqnfkUH5xwp0ZxKNgUPFed8oHR40= X-Google-Smtp-Source: AGHT+IEFp1LYkqGFtNbmR422YJnqFFI8fTBy63QxTHUvjzRIoP7Ald+8kd/ogLf8DJDRFadAPDh/XaorXLQ= X-Received: from zagreus.c.googlers.com ([fda3:e722:ac3:cc00:7f:e700:c0a8:5c37]) (user=seanjc job=sendgmr) by 2002:a63:950c:0:b0:565:e2cd:c9e1 with SMTP id p12-20020a63950c000000b00565e2cdc9e1mr588044pgd.11.1692211037524; Wed, 16 Aug 2023 11:37:17 -0700 (PDT) Date: Wed, 16 Aug 2023 11:37:16 -0700 In-Reply-To: <20230802051700.52321-2-likexu@tencent.com> Mime-Version: 1.0 References: <20230802051700.52321-1-likexu@tencent.com> <20230802051700.52321-2-likexu@tencent.com> Message-ID: Subject: Re: [PATCH v2 1/2] KVM: eventfd: Fix NULL deref irqbypass producer From: Sean Christopherson To: Like Xu Cc: Alex Williamson , Paolo Bonzini , linux-kernel@vger.kernel.org, kvm@vger.kernel.org Content-Type: text/plain; charset="us-ascii" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Aug 02, 2023, Like Xu wrote: > From: Like Xu > > 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 > Signed-off-by: Like Xu > Reviewed-by: Alex Williamson > --- > 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))