diff mbox series

net: ethernet: fix NULL dereference in nixge_recv()

Message ID 20241211083424.2580563-1-make_ruc2021@163.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: ethernet: fix NULL dereference in nixge_recv() | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 2 this patch: 2
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-12-11--15-00 (tests: 764)

Commit Message

Ma Ke Dec. 11, 2024, 8:34 a.m. UTC 
In function nixge_recv() dereference of NULL pointer priv->rx_bd_v is
possible for the case of its allocation failure in netdev_priv(ndev).

Move while() loop with priv->rx_bd_v dereference under the check for
its validity.

Cc: stable@vger.kernel.org
Fixes: 492caffa8a1a ("net: ethernet: nixge: Add support for National Instruments XGE netdev")
Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
 drivers/net/ethernet/ni/nixge.c | 86 ++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 43 deletions(-)

Comments

Przemek Kitszel Dec. 11, 2024, 9:28 a.m. UTC | #1 
On 12/11/24 09:34, Ma Ke wrote:
> In function nixge_recv() dereference of NULL pointer priv->rx_bd_v is
> possible for the case of its allocation failure in netdev_priv(ndev).

please be more precise about the case, rx_bd_v is not set during
netdev(w/priv) allocation

the embedded priv part is allocated together with the netdev, in the
.probe()

> 
> Move while() loop with priv->rx_bd_v dereference under the check for
> its validity.

this style has some benefits, but in the kernel we prefer the early
return:
	if (!priv->rx_bd_v)
		return 0;

instead of touching a whole function

> 
> Cc: stable@vger.kernel.org
> Fixes: 492caffa8a1a ("net: ethernet: nixge: Add support for National Instruments XGE netdev")
> Signed-off-by: Ma Ke <make_ruc2021@163.com>
> ---
>   drivers/net/ethernet/ni/nixge.c | 86 ++++++++++++++++-----------------
>   1 file changed, 43 insertions(+), 43 deletions(-)
Andrew Lunn Dec. 11, 2024, 1:57 p.m. UTC | #2 
On Wed, Dec 11, 2024 at 04:34:24PM +0800, Ma Ke wrote:
> In function nixge_recv() dereference of NULL pointer priv->rx_bd_v is
> possible for the case of its allocation failure in netdev_priv(ndev).
> 
> Move while() loop with priv->rx_bd_v dereference under the check for
> its validity.
> 
> Cc: stable@vger.kernel.org
> Fixes: 492caffa8a1a ("net: ethernet: nixge: Add support for National Instruments XGE netdev")
> Signed-off-by: Ma Ke <make_ruc2021@163.com>
> ---
>  drivers/net/ethernet/ni/nixge.c | 86 ++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
> index 230d5ff99dd7..2935ffd62e2a 100644
> --- a/drivers/net/ethernet/ni/nixge.c
> +++ b/drivers/net/ethernet/ni/nixge.c
> @@ -603,64 +603,64 @@ static int nixge_recv(struct net_device *ndev, int budget)

Is this a hot path function? It appears to be used for every single
packet.

Is it possible to check for allocation failures outside of the hot
path? Is priv->rx_bd_v allocated once during probe? If so, fail the
probe.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index 230d5ff99dd7..2935ffd62e2a 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -603,64 +603,64 @@  static int nixge_recv(struct net_device *ndev, int budget)
 	u32 size = 0;
 
 	cur_p = &priv->rx_bd_v[priv->rx_bd_ci];
+	if (priv->rx_bd_v) {
+		while ((cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK &&
+			budget > packets)) {
+			tail_p = priv->rx_bd_p + sizeof(*priv->rx_bd_v) *
+				 priv->rx_bd_ci;
 
-	while ((cur_p->status & XAXIDMA_BD_STS_COMPLETE_MASK &&
-		budget > packets)) {
-		tail_p = priv->rx_bd_p + sizeof(*priv->rx_bd_v) *
-			 priv->rx_bd_ci;
+			skb = (struct sk_buff *)(uintptr_t)
+				nixge_hw_dma_bd_get_addr(cur_p, sw_id_offset);
 
-		skb = (struct sk_buff *)(uintptr_t)
-			nixge_hw_dma_bd_get_addr(cur_p, sw_id_offset);
+			length = cur_p->status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK;
+			if (length > NIXGE_MAX_JUMBO_FRAME_SIZE)
+				length = NIXGE_MAX_JUMBO_FRAME_SIZE;
 
-		length = cur_p->status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK;
-		if (length > NIXGE_MAX_JUMBO_FRAME_SIZE)
-			length = NIXGE_MAX_JUMBO_FRAME_SIZE;
+			dma_unmap_single(ndev->dev.parent,
+					 nixge_hw_dma_bd_get_addr(cur_p, phys),
+					 NIXGE_MAX_JUMBO_FRAME_SIZE,
+					 DMA_FROM_DEVICE);
 
-		dma_unmap_single(ndev->dev.parent,
-				 nixge_hw_dma_bd_get_addr(cur_p, phys),
-				 NIXGE_MAX_JUMBO_FRAME_SIZE,
-				 DMA_FROM_DEVICE);
+			skb_put(skb, length);
 
-		skb_put(skb, length);
+			skb->protocol = eth_type_trans(skb, ndev);
+			skb_checksum_none_assert(skb);
 
-		skb->protocol = eth_type_trans(skb, ndev);
-		skb_checksum_none_assert(skb);
+			/* For now mark them as CHECKSUM_NONE since
+			 * we don't have offload capabilities
+			 */
+			skb->ip_summed = CHECKSUM_NONE;
 
-		/* For now mark them as CHECKSUM_NONE since
-		 * we don't have offload capabilities
-		 */
-		skb->ip_summed = CHECKSUM_NONE;
+			napi_gro_receive(&priv->napi, skb);
 
-		napi_gro_receive(&priv->napi, skb);
+			size += length;
+			packets++;
 
-		size += length;
-		packets++;
+			new_skb = netdev_alloc_skb_ip_align(ndev,
+							    NIXGE_MAX_JUMBO_FRAME_SIZE);
+			if (!new_skb)
+				return packets;
 
-		new_skb = netdev_alloc_skb_ip_align(ndev,
-						    NIXGE_MAX_JUMBO_FRAME_SIZE);
-		if (!new_skb)
-			return packets;
-
-		cur_phys = dma_map_single(ndev->dev.parent, new_skb->data,
-					  NIXGE_MAX_JUMBO_FRAME_SIZE,
-					  DMA_FROM_DEVICE);
-		if (dma_mapping_error(ndev->dev.parent, cur_phys)) {
-			/* FIXME: bail out and clean up */
-			netdev_err(ndev, "Failed to map ...\n");
+			cur_phys = dma_map_single(ndev->dev.parent, new_skb->data,
+						  NIXGE_MAX_JUMBO_FRAME_SIZE,
+						  DMA_FROM_DEVICE);
+			if (dma_mapping_error(ndev->dev.parent, cur_phys)) {
+				/* FIXME: bail out and clean up */
+				netdev_err(ndev, "Failed to map ...\n");
+			}
+			nixge_hw_dma_bd_set_phys(cur_p, cur_phys);
+			cur_p->cntrl = NIXGE_MAX_JUMBO_FRAME_SIZE;
+			cur_p->status = 0;
+			nixge_hw_dma_bd_set_offset(cur_p, (uintptr_t)new_skb);
+
+			++priv->rx_bd_ci;
+			priv->rx_bd_ci %= RX_BD_NUM;
+			cur_p = &priv->rx_bd_v[priv->rx_bd_ci];
 		}
-		nixge_hw_dma_bd_set_phys(cur_p, cur_phys);
-		cur_p->cntrl = NIXGE_MAX_JUMBO_FRAME_SIZE;
-		cur_p->status = 0;
-		nixge_hw_dma_bd_set_offset(cur_p, (uintptr_t)new_skb);
-
-		++priv->rx_bd_ci;
-		priv->rx_bd_ci %= RX_BD_NUM;
-		cur_p = &priv->rx_bd_v[priv->rx_bd_ci];
 	}
 
 	ndev->stats.rx_packets += packets;
 	ndev->stats.rx_bytes += size;
-
 	if (tail_p)
 		nixge_dma_write_desc_reg(priv, XAXIDMA_RX_TDESC_OFFSET, tail_p);