diff mbox series

[RFC,net] ps3/gelic: Fix possible NULL pointer dereference

Message ID 20240221-ps3-gelic-null-deref-v1-1-f4fe159c7cb0@kernel.org
State New
Headers
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

Dan Carpenter Feb. 21, 2024, 6:32 p.m. UTC | #1 
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
Geoff Levand Feb. 22, 2024, 6:46 a.m. UTC | #2 
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
Geoff Levand Feb. 22, 2024, 7:23 a.m. UTC | #3 
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>
Geert Uytterhoeven Feb. 22, 2024, 8:45 a.m. UTC | #4 
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 mbox series

Patch

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;