Message ID | 20240221-ps3-gelic-null-deref-v1-1-f4fe159c7cb0@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-75148-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp1164387dyc; Wed, 21 Feb 2024 09:00:17 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCWwYuyEvewByYIHNAFjpgN1ll0OHmhDkmVK5CkRrQdEt1DG5GxR7fm8xNVwox3JvvEV1ZORKtmGiG6sFn30232Z/DS5Lw== X-Google-Smtp-Source: AGHT+IFFH39IOIi6Q6fzUqSInemfSOpyocj9mKDsLsPOZGg3U3yaQdjV1gwPzm0Z1YPvx6/LRCbu X-Received: by 2002:a05:6a20:ce48:b0:1a0:726a:6e44 with SMTP id id8-20020a056a20ce4800b001a0726a6e44mr17804923pzb.3.1708534817390; Wed, 21 Feb 2024 09:00:17 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1708534817; cv=pass; d=google.com; s=arc-20160816; b=VZwqMpnUC5NJ+rbi1N+0tCW8k7mOXlu9fzprvUrkj3yI1mHpyQWWWns5LvY9KsKzLo i5NsparKZtzbzR44My7vId2yGhcvLixJ8Lb5LrC0g1xJuto4M3SY4xH66qjHY8EI/NBA LF3ivcGWaDIzKUFpIw/1nytbHiS3RzWYIEN5AQc2yukFk5ushYbMlRXpohZkYFiwFeuQ OEwQq5hYdYDdEd0TUYtTo3Cslwxf0442J7n4ZpOFzlmUsJ/qTL/7K4sC4NBddN18HqgC N6FAtKKloly+QVCQaa9dvMbdF5WD+2TkCW+/HdjXOgd1JOvHM+wcTJ35L9ZOxeyQPff3 xg+A== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:message-id:content-transfer-encoding:mime-version :list-unsubscribe:list-subscribe:list-id:precedence:subject:date :from:dkim-signature; bh=/aPLjhAp2OnjUIAlMGTx4pBfyJgqhDB1Lsyk868JELQ=; fh=hNMxi27uBPco55BpzWkZsv+UuH2ukMfRfsKLlXvr0UM=; b=fiSolF5U77oA85LTpSw/ErOWyIL3fxJm8BWPzZ+iiKUModiZqJyWcmU3YxgqQkH2gw twfV2Ap8CNTGy7etHanYv7ZtWROB4Z6E2ns3248kibC5Rmx5nar3oj62+ONCDgJsJKxr ChIFxOjPuxSDzH20Q1+qachUNv+0YxHgYk3wtiTLjv97enJ2lD7k8vRBi7agXaT8I62R 5nQdvDKl0WtfKJpQJwY0U2RFxi3soAiQL6QUy1LbX1vBmAwMt9Bzamgpv3fpjbEHWUFC G/VzTzTnY5AwuC19WaBzZkPRSb7mIv1eeOoFZHjtBEzM/v2t4LVbADwBuYA/hKPFmZpV zNxA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=G6y2XApx; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-75148-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-75148-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [2604:1380:40f1:3f00::1]) by mx.google.com with ESMTPS id b17-20020a639311000000b0058986c61bb6si8545026pge.706.2024.02.21.09.00.16 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Feb 2024 09:00:17 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-75148-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=G6y2XApx; arc=pass (i=1 dkim=pass dkdomain=kernel.org); spf=pass (google.com: domain of linux-kernel+bounces-75148-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-75148-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id A1ADBB23A5E for <ouuuleilei@gmail.com>; Wed, 21 Feb 2024 16:57:19 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id E557783CA4; Wed, 21 Feb 2024 16:57:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="G6y2XApx" Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 65AE97FBC4; Wed, 21 Feb 2024 16:57:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708534621; cv=none; b=qXhanSMZXCEG0EzDk9s9wxXDBjfH/uqbOrtL3k3pZFjPEhHKX1NQW+a2z7HdGwlm0Eb52W47eMLFAjatHRpd+JzSalZ2X6fMf8qivlZcmdGwBxi+42idHFXfCY9vZVXKyGpQ8ZkdSUZ1y6IEJb/X4TsRCf8PipT55mjAgX+IuQI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708534621; c=relaxed/simple; bh=fKXceB0Gk5Qi2BTh8FnrZ+MT+hE3FOYlCf2QUIU2XXQ=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:To:Cc; b=FCs5+dJlzdF7G83G8MKneF405apamR9cipsdX344vJkUwA9kUmMQTGIKoRh3yJ50X7W3R4pwC8AbdbTSOL+hbarskB7HorbrvqhsLC8WAYEydpS+UDHJpn9RlOE4BL8RYh0AFtZf9G68fguU3znm0P3AIceu1aukiwTxZ1/eXJ4= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=G6y2XApx; arc=none smtp.client-ip=10.30.226.201 Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2284BC433F1; Wed, 21 Feb 2024 16:56:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1708534620; bh=fKXceB0Gk5Qi2BTh8FnrZ+MT+hE3FOYlCf2QUIU2XXQ=; h=From:Date:Subject:To:Cc:From; b=G6y2XApxOSs9SZl79EQohPRj/X5MpyJELyXCc2UtgTxkX488zdh7HWdDR+lyX0unX CUJ3F7ZZbY3Vx2bdoT0jpYzMKE1H9IUANbSkmtC/wtbJFE8gw5IkjPu/GucJ+ibf6Y 88tDFcattShPJ7M6gdeISV3hDqCuzLs2iKAmubTOYAxlgjmAHL/Ure//BNYZT+JXVD ugZUI3LvAbt5KssDhcPEKqu2br9DI5l/QW1EVY8L8rhcEEHlMg48HzlCALNApQfYyt gpH+W9lLfViXuIrXKtO70sqLRd5E8bI/RFu4RbsPzbktT1Gd1Ec0M0V2ZPWQjOfv8F NCve65P6JPhpA== From: Simon Horman <horms@kernel.org> Date: Wed, 21 Feb 2024 16:56:47 +0000 Subject: [PATCH RFC net] ps3/gelic: Fix possible NULL pointer dereference Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20240221-ps3-gelic-null-deref-v1-1-f4fe159c7cb0@kernel.org> X-B4-Tracking: v=1; b=H4sIAE4r1mUC/x2MQQqDMBQFrxL+2g9JWkTcCh7ArXRhk6d+CFESW wri3RtczsDMSRlJkKlVJyV8JcsWC5hKkVunuIDFFyar7VNba3jPD14QxHH8hMAeCTM32ji42jS Tf1NJ9yLld29HGvpORRz0uq4/qOxVpW8AAAA= To: Geoff Levand <geoff@infradead.org>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com> Cc: Michael Ellerman <mpe@ellerman.id.au>, Nicholas Piggin <npiggin@gmail.com>, Christophe Leroy <christophe.leroy@csgroup.eu>, "Aneesh Kumar K.V" <aneesh.kumar@kernel.org>, "Naveen N. Rao" <naveen.n.rao@linux.ibm.com>, Jeff Garzik <jeff@garzik.org>, Dan Carpenter <dan.carpenter@linaro.org>, netdev@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org X-Mailer: b4 0.12.3 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791528604912608558 X-GMAIL-MSGID: 1791528604912608558 |
Series |
[RFC,net] ps3/gelic: Fix possible NULL pointer dereference
|
|
Commit Message
Simon Horman
Feb. 21, 2024, 4:56 p.m. UTC
Fix possible NULL pointer dereference in gelic_card_release_tx_chain()
The cited commit introduced a netdev variable to
gelic_card_release_tx_chain() which is set unconditionally on each
iteration of a for loop.
It is set to the value of tx_chain->tail->skb->dev. However, in some
cases it is assumed that tx_chain->tail->skb may be NULL. And if that
occurs, setting netdev will cause a NULl pointer dereference.
Given the age of this code I do wonder if this can occur in practice.
But to be on the safe side this patch assumes that it can and aims to
avoid the dereference in the case where tx_chain->tail->skb may be NULL.
Flagged by Smatch.
Compile tested only.
Fixes: 589866f9f1cb ("PS3: gelic: Add support for dual network interface")
Signed-off-by: Simon Horman <horms@kernel.org>
---
drivers/net/ethernet/toshiba/ps3_gelic_net.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
Comments
This driver is PPC so I have never looked at the code before. I noticed another issue that was introduced last December in commit 3ce4f9c3fbb3 ("net/ps3_gelic_net: Add gelic_descr structures"). net/ethernet/toshiba/ps3_gelic_net.c 375 static int gelic_descr_prepare_rx(struct gelic_card *card, 376 struct gelic_descr *descr) 377 { 378 static const unsigned int rx_skb_size = 379 ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) + 380 GELIC_NET_RXBUF_ALIGN - 1; 381 dma_addr_t cpu_addr; 382 int offset; 383 384 if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) 385 dev_info(ctodev(card), "%s: ERROR status\n", __func__); 386 387 descr->skb = netdev_alloc_skb(*card->netdev, rx_skb_size); 388 if (!descr->skb) { 389 descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't touch memory */ 390 return -ENOMEM; 391 } 392 descr->hw_regs.dmac_cmd_status = 0; 393 descr->hw_regs.result_size = 0; 394 descr->hw_regs.valid_size = 0; 395 descr->hw_regs.data_error = 0; 396 descr->hw_regs.payload.dev_addr = 0; 397 descr->hw_regs.payload.size = 0; 398 descr->skb = NULL; ^^^^^^^^^^^^^^^^^^ NULL 399 400 offset = ((unsigned long)descr->skb->data) & ^^^^^^^^^^^^ Dereferenced here. 401 (GELIC_NET_RXBUF_ALIGN - 1); 402 if (offset) 403 skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset); 404 /* io-mmu-map the skb */ 405 cpu_addr = dma_map_single(ctodev(card), descr->skb->data, 406 GELIC_NET_MAX_FRAME, DMA_FROM_DEVICE); regards, dan carpenter
On 2/22/24 03:32, Dan Carpenter wrote: > This driver is PPC so I have never looked at the code before. I noticed > another issue that was introduced last December in commit 3ce4f9c3fbb3 > ("net/ps3_gelic_net: Add gelic_descr structures"). > > net/ethernet/toshiba/ps3_gelic_net.c .. > 375 static int gelic_descr_prepare_rx(struct gelic_card *card, > 376 struct gelic_descr *descr) > 398 descr->skb = NULL; > ^^^^^^^^^^^^^^^^^^ > NULL > > 399 > 400 offset = ((unsigned long)descr->skb->data) & > ^^^^^^^^^^^^ > Dereferenced here. There is a fix, see '[PATCH v6 net] ps3/gelic: Fix SKB allocation': https://lore.kernel.org/netdev/20240221172824.GD722610@kernel.org/T/ -Geoff
Hi Simon, On 2/22/24 01:56, Simon Horman wrote: > Fix possible NULL pointer dereference in gelic_card_release_tx_chain() > > The cited commit introduced a netdev variable to > gelic_card_release_tx_chain() which is set unconditionally on each > iteration of a for loop. > > It is set to the value of tx_chain->tail->skb->dev. However, in some > cases it is assumed that tx_chain->tail->skb may be NULL. And if that > occurs, setting netdev will cause a NULl pointer dereference. > > Given the age of this code I do wonder if this can occur in practice. > But to be on the safe side this patch assumes that it can and aims to > avoid the dereference in the case where tx_chain->tail->skb may be NULL. After 17+ years I never hit this, and never heard of anyone hitting it... > Flagged by Smatch. > Compile tested only. Thanks for 'fixing' this. Acked-by: Geoff Levand <geoff@infradead.org>
Hi Simon, On Wed, Feb 21, 2024 at 5:57 PM Simon Horman <horms@kernel.org> wrote: > Fix possible NULL pointer dereference in gelic_card_release_tx_chain() > > The cited commit introduced a netdev variable to > gelic_card_release_tx_chain() which is set unconditionally on each > iteration of a for loop. > > It is set to the value of tx_chain->tail->skb->dev. However, in some > cases it is assumed that tx_chain->tail->skb may be NULL. And if that > occurs, setting netdev will cause a NULl pointer dereference. Thanks for your patch! > Given the age of this code I do wonder if this can occur in practice. > But to be on the safe side this patch assumes that it can and aims to > avoid the dereference in the case where tx_chain->tail->skb may be NULL. The compiler may also lazy-load netdev until it's actually used, avoiding the crash? > Fixes: 589866f9f1cb ("PS3: gelic: Add support for dual network interface") > Signed-off-by: Simon Horman <horms@kernel.org> Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Gr{oetje,eeting}s, Geert
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c index d5b75af163d3..f03489799f5d 100644 --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c @@ -549,14 +549,13 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop) { struct gelic_descr_chain *tx_chain; enum gelic_descr_dma_status status; - struct net_device *netdev; int release = 0; for (tx_chain = &card->tx_chain; tx_chain->head != tx_chain->tail && tx_chain->tail; tx_chain->tail = tx_chain->tail->next) { status = gelic_descr_get_status(tx_chain->tail); - netdev = tx_chain->tail->skb->dev; + switch (status) { case GELIC_DESCR_DMA_RESPONSE_ERROR: case GELIC_DESCR_DMA_PROTECTION_ERROR: @@ -566,11 +565,14 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop) "%s: forcing end of tx descriptor " \ "with status %x\n", __func__, status); - netdev->stats.tx_dropped++; + tx_chain->tail->skb->dev->stats.tx_dropped++; break; case GELIC_DESCR_DMA_COMPLETE: if (tx_chain->tail->skb) { + struct net_device *netdev; + + netdev = tx_chain->tail->skb->dev; netdev->stats.tx_packets++; netdev->stats.tx_bytes += tx_chain->tail->skb->len;