| Message ID | 20240221-ps3-gelic-null-deref-v1-1-f4fe159c7cb0@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] ps3/gelic: Fix possible NULL pointer dereference | expand |
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;
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(-)