Message ID | 20230613192220.159407-1-pchelkin@ispras.ru |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp786375vqr; Tue, 13 Jun 2023 12:47:35 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ59Cyq+yHAkc/7xf7+nB16WHR18vvXpt3znjSFeNmSTbtfwOXSIdumBK/CjFY5feNRGJd59 X-Received: by 2002:a17:906:dc8e:b0:968:4ce9:677a with SMTP id cs14-20020a170906dc8e00b009684ce9677amr15205903ejc.38.1686685655238; Tue, 13 Jun 2023 12:47:35 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686685655; cv=none; d=google.com; s=arc-20160816; b=TZpHANiug/SQkfPgafnDRu6T/EgNo7AF91/LOUxsLw1x3OdHUY3cKtvQfUo1mFW3m9 SpY3CSUPywDILBLxiYZdsWzPpZAUQomhLJ6gS+HAAoOEO1e1Y5zxWBUwg0Vtm5h5pTCW TqqqkqPNGBEtC/Xb7MkAKhg3eXKDtwhKCrCeQYRVPIbCNObqpQwdmHveIK5/mQBqv1Xy oLA5+sAuXZV+tiCKCd1Hk3toNiTTHvFz3zi5G253D1R/OmA9M7BIkwSnCrm1jY3axRDd /SY2AILHetNooqAW9YvuKlqQHB1/MA3vLN7eLTMEBPxgV/2a3Ip/cDYTQrs/+RfEglsp f0Yg== 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 :message-id:date:subject:cc:to:from:dkim-signature:dkim-filter; bh=hIRs6Uzx3zjkKCTmplY/Pi+dHaV4Bk/epLmdjrJuCpU=; b=xzbIBBL56Rf4Uh9aCpWHJ4IY44THoOmjSmGl0uxRNomRCuS2UTkKKIapBI8P52502y OihjxUGQi8ay/NZ7hfkj0aDxcGIZAnW2GMq5NZECkZRb2jxJcZ0ev2m0j/h8SLIj1sop cNpD9QR54IK2yv4WdbB4LY70166rRl+2CisaNWBE8GgY/7HVN7khlZZ03sDsj8FmFULO WEF8nbEUUthjeS2Zgqi4YVxEi2E4VvzfBJA380yPWDysPxK96UN06VWEMB/sor6lNlr/ uaXOH7UO3CsAJZnMW/4y1mwbK2gbXDP/36bNCkhmQ3PaxBAz0B+2FHfkMR19f6JfD804 NoXg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ispras.ru header.s=default header.b=gk5PMbPc; 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=NONE dis=NONE) header.from=ispras.ru Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id v24-20020a170906339800b009787062562esi6845394eja.585.2023.06.13.12.47.10; Tue, 13 Jun 2023 12:47:35 -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=@ispras.ru header.s=default header.b=gk5PMbPc; 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=NONE dis=NONE) header.from=ispras.ru Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231401AbjFMTXS (ORCPT <rfc822;lekhanya01809@gmail.com> + 99 others); Tue, 13 Jun 2023 15:23:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60114 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229504AbjFMTXR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 13 Jun 2023 15:23:17 -0400 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.84]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 352D01A5; Tue, 13 Jun 2023 12:23:14 -0700 (PDT) Received: from fpc.intra.ispras.ru (unknown [10.10.165.7]) by mail.ispras.ru (Postfix) with ESMTPSA id 4AFEC40737DA; Tue, 13 Jun 2023 19:23:09 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 mail.ispras.ru 4AFEC40737DA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ispras.ru; s=default; t=1686684189; bh=hIRs6Uzx3zjkKCTmplY/Pi+dHaV4Bk/epLmdjrJuCpU=; h=From:To:Cc:Subject:Date:From; b=gk5PMbPcjnyrxDmuuKVB1Jfl6MY4re3uzov615Wm4y+5VHIIZjhshadNqXpi2PaQS qgNE+00grlAlcoepwp3VpOZgku5iOaibXnSIX1FOZ8sAP+BuxoSuvW7PMWHaV2/GRj WtvS7APzt8bZZ8lUjigxkcDuaK9WBi2l9O/mv5AA= From: Fedor Pchelkin <pchelkin@ispras.ru> To: "David S. Miller" <davem@davemloft.net> Cc: Fedor Pchelkin <pchelkin@ispras.ru>, Sabrina Dubroca <sd@queasysnail.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Raed Salem <raeds@nvidia.com>, Lior Nahmanson <liorna@nvidia.com>, Saeed Mahameed <saeedm@nvidia.com>, Hannes Frederic Sowa <hannes@stressinduktion.org>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Alexey Khoroshilov <khoroshilov@ispras.ru>, lvc-project@linuxtesting.org Subject: [PATCH] net: macsec: fix double free of percpu stats Date: Tue, 13 Jun 2023 22:22:20 +0300 Message-Id: <20230613192220.159407-1-pchelkin@ispras.ru> X-Mailer: git-send-email 2.34.1 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,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768618097674973086?= X-GMAIL-MSGID: =?utf-8?q?1768618097674973086?= |
Series |
net: macsec: fix double free of percpu stats
|
|
Commit Message
Fedor Pchelkin
June 13, 2023, 7:22 p.m. UTC
Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and
macsec->stats on some of the memory allocation failure paths. However, the
net_device is already registered to that moment: in macsec_newlink(), just
before calling macsec_add_dev(). This means that during unregister process
its priv_destructor - macsec_free_netdev() - will be called and will free
the stats again.
Remove freeing percpu stats inside macsec_add_dev() because
macsec_free_netdev() will correctly free the already allocated ones. The
pointers to unallocated stats stay NULL, and free_percpu() treats that
correctly.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 0a28bfd4971f ("net/macsec: Add MACsec skb_metadata_dst Tx Data path support")
Fixes: c09440f7dcb3 ("macsec: introduce IEEE 802.1AE driver")
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
drivers/net/macsec.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
Comments
On Tue, 13 Jun 2023 22:22:20 +0300 Fedor Pchelkin wrote: > Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and > macsec->stats on some of the memory allocation failure paths. However, the > net_device is already registered to that moment: in macsec_newlink(), just > before calling macsec_add_dev(). This means that during unregister process > its priv_destructor - macsec_free_netdev() - will be called and will free > the stats again. > > Remove freeing percpu stats inside macsec_add_dev() because > macsec_free_netdev() will correctly free the already allocated ones. The > pointers to unallocated stats stay NULL, and free_percpu() treats that > correctly. What prevents the device from being opened and used before macsec_add_dev() has finished? I think we need a fix which would move this code before register_netdev(), instead :(
2023-06-13, 20:01:50 -0700, Jakub Kicinski wrote: > On Tue, 13 Jun 2023 22:22:20 +0300 Fedor Pchelkin wrote: > > Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and > > macsec->stats on some of the memory allocation failure paths. However, the > > net_device is already registered to that moment: in macsec_newlink(), just > > before calling macsec_add_dev(). This means that during unregister process > > its priv_destructor - macsec_free_netdev() - will be called and will free > > the stats again. > > > > Remove freeing percpu stats inside macsec_add_dev() because > > macsec_free_netdev() will correctly free the already allocated ones. The > > pointers to unallocated stats stay NULL, and free_percpu() treats that > > correctly. > > What prevents the device from being opened and used before > macsec_add_dev() has finished? I think we need a fix which > would move this code before register_netdev(), instead :( Can the device be opened in parallel? We're under rtnl here. If we want to move that code, then we'll also have to move the eth_hw_addr_inherit call that's currently in macsec's ndo_init: in case the user didn't give an SCI, we have to make it up based on the device's mac address (dev_to_sci(dev, ...)), whether it's set by the user or inherited. I can't remember if I had a good reason to put the inherit in ndo_init.
On Wed, 14 Jun 2023 14:26:14 +0200 Sabrina Dubroca wrote: > > What prevents the device from being opened and used before > > macsec_add_dev() has finished? I think we need a fix which > > would move this code before register_netdev(), instead :( > > Can the device be opened in parallel? We're under rtnl here. > > If we want to move that code, then we'll also have to move the > eth_hw_addr_inherit call that's currently in macsec's ndo_init: in > case the user didn't give an SCI, we have to make it up based on the > device's mac address (dev_to_sci(dev, ...)), whether it's set by the > user or inherited. I can't remember if I had a good reason to put the > inherit in ndo_init. Ah, you're right, this is a link creation path.
On Wed, Jun 14, 2023 at 09:01:26AM -0700, Jakub Kicinski wrote: > On Wed, 14 Jun 2023 14:26:14 +0200 Sabrina Dubroca wrote: > > > What prevents the device from being opened and used before > > > macsec_add_dev() has finished? I think we need a fix which > > > would move this code before register_netdev(), instead :( > > > > Can the device be opened in parallel? We're under rtnl here. > > > > If we want to move that code, then we'll also have to move the > > eth_hw_addr_inherit call that's currently in macsec's ndo_init: in > > case the user didn't give an SCI, we have to make it up based on the > > device's mac address (dev_to_sci(dev, ...)), whether it's set by the > > user or inherited. I can't remember if I had a good reason to put the > > inherit in ndo_init. > > Ah, you're right, this is a link creation path. My reply probably won't give any new information now but if the code of macsec_add_dev() and the parts from ndo_init it depends on which Sabrina mentioned would be moved before registering netdev then the problem will go away on its own. Is it worth moving that code if rtnl_lock is held? Maybe it will be more persistent to initialize the device for as maximum as possible before calling register_netdevice()? Overall, it all depends on the reasons why the code was implemented so initially.
2023-06-14, 23:17:14 +0300, Fedor Pchelkin wrote: > On Wed, Jun 14, 2023 at 09:01:26AM -0700, Jakub Kicinski wrote: > > On Wed, 14 Jun 2023 14:26:14 +0200 Sabrina Dubroca wrote: > > > > What prevents the device from being opened and used before > > > > macsec_add_dev() has finished? I think we need a fix which > > > > would move this code before register_netdev(), instead :( > > > > > > Can the device be opened in parallel? We're under rtnl here. > > > > > > If we want to move that code, then we'll also have to move the > > > eth_hw_addr_inherit call that's currently in macsec's ndo_init: in > > > case the user didn't give an SCI, we have to make it up based on the > > > device's mac address (dev_to_sci(dev, ...)), whether it's set by the > > > user or inherited. I can't remember if I had a good reason to put the > > > inherit in ndo_init. > > > > Ah, you're right, this is a link creation path. > > My reply probably won't give any new information now but if the code of > macsec_add_dev() and the parts from ndo_init it depends on which Sabrina > mentioned would be moved before registering netdev then the problem will > go away on its own. > > Is it worth moving that code if rtnl_lock is held? Maybe it will be more > persistent to initialize the device for as maximum as possible before > calling register_netdevice()? Overall, it all depends on the reasons why > the code was implemented so initially. It's been 7 years... your guess is about as good as mine :/ I wouldn't bother reshuffling the device creation code just to make the handling of rare failures a bit nicer.
On Wed, 14 Jun 2023 23:15:03 +0200 Sabrina Dubroca wrote: > It's been 7 years... your guess is about as good as mine :/ > > I wouldn't bother reshuffling the device creation code just to make > the handling of rare failures a bit nicer. Would you be willing to venture a review tag?
2023-06-14, 23:02:39 -0700, Jakub Kicinski wrote: > On Wed, 14 Jun 2023 23:15:03 +0200 Sabrina Dubroca wrote: > > It's been 7 years... your guess is about as good as mine :/ > > > > I wouldn't bother reshuffling the device creation code just to make > > the handling of rare failures a bit nicer. > > Would you be willing to venture a review tag? Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Tue, 13 Jun 2023 22:22:20 +0300 you wrote: > Inside macsec_add_dev() we free percpu macsec->secy.tx_sc.stats and > macsec->stats on some of the memory allocation failure paths. However, the > net_device is already registered to that moment: in macsec_newlink(), just > before calling macsec_add_dev(). This means that during unregister process > its priv_destructor - macsec_free_netdev() - will be called and will free > the stats again. > > [...] Here is the summary with links: - net: macsec: fix double free of percpu stats https://git.kernel.org/netdev/net/c/0c0cf3db83f8 You are awesome, thank you!
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index 3427993f94f7..984dfa5d6c11 100644 --- a/drivers/net/macsec.c +++ b/drivers/net/macsec.c @@ -3997,17 +3997,15 @@ static int macsec_add_dev(struct net_device *dev, sci_t sci, u8 icv_len) return -ENOMEM; secy->tx_sc.stats = netdev_alloc_pcpu_stats(struct pcpu_tx_sc_stats); - if (!secy->tx_sc.stats) { - free_percpu(macsec->stats); + if (!secy->tx_sc.stats) return -ENOMEM; - } secy->tx_sc.md_dst = metadata_dst_alloc(0, METADATA_MACSEC, GFP_KERNEL); - if (!secy->tx_sc.md_dst) { - free_percpu(secy->tx_sc.stats); - free_percpu(macsec->stats); + if (!secy->tx_sc.md_dst) + /* macsec and secy percpu stats will be freed when unregistering + * net_device in macsec_free_netdev() + */ return -ENOMEM; - } if (sci == MACSEC_UNDEF_SCI) sci = dev_to_sci(dev, MACSEC_PORT_ES);