[PATCH net] hinic3: bound the Rx fragment count to MAX_SKB_FRAGS

[PATCH net] hinic3: bound the Rx fragment count to MAX_SKB_FRAGS

[Yehyeong Lee]

hinic3_rx_poll() takes the packet length from the Rx completion
descriptor and turns it into a fragment count:

      pkt_len = RQ_CQE_SGE_GET(vlan_len, LEN);
      sge_num = pkt_len >> rxq->buf_len_shift ...

RQ_CQE_SGE_LEN_MASK is a 16-bit field, so pkt_len is up to 65535 and
sge_num up to 32 with the default 2048-byte buffers.  packaging_skb()
then calls skb_add_rx_frag() sge_num times without checking the count
against MAX_SKB_FRAGS, so a device reporting a length above
MAX_SKB_FRAGS * buf_len writes fragment descriptors past the end of the
skb's shared info array.

Driving the receive path with an out-of-range length reports under
KASAN:

      BUG: KASAN: slab-out-of-bounds in hinic3_fetch_rx_buffer [hinic3]
      Write of size 8 ...
      The buggy address belongs to the object ...
       which belongs to the cache skbuff_small_head of size 704

The length is chosen by the device and an skb cannot hold more than
MAX_SKB_FRAGS fragments, so drop a frame that needs more instead of
overrunning the array.  The buffers it claimed are already posted on the
ring, so return them to the page pool and advance the consumer index
over them to keep it in step with the device.

Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 .../net/ethernet/huawei/hinic3/hinic3_rx.c    | 42 +++++++++++++++++--
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
index 309ab59013797..974dab51fa425 100644
--- a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
+++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
@@ -275,6 +275,34 @@ static u32 hinic3_get_sge_num(struct hinic3_rxq *rxq, u32 pkt_len)
 	return sge_num;
 }
 
+/* The device reported a frame that needs more fragments than an skb can
+ * carry.  Give the buffers it claimed back to the pool and advance the ring
+ * over them, so the consumer index stays in step with the device.
+ */
+static void hinic3_discard_rx_pkt(struct hinic3_rxq *rxq, u32 sge_num)
+{
+	u32 sw_ci = rxq->cons_idx & rxq->q_mask;
+	u32 i;
+
+	for (i = 0; i < sge_num; i++) {
+		struct hinic3_rx_info *rx_info = &rxq->rx_info[sw_ci];
+
+		if (rx_info->page) {
+			page_pool_put_full_page(rxq->page_pool, rx_info->page,
+						false);
+			rx_info->page = NULL;
+		}
+		sw_ci = (sw_ci + 1) & rxq->q_mask;
+	}
+
+	rxq->cons_idx += sge_num;
+	rxq->delta += sge_num;
+
+	u64_stats_update_begin(&rxq->rxq_stats.syncp);
+	rxq->rxq_stats.dropped++;
+	u64_stats_update_end(&rxq->rxq_stats.syncp);
+}
+
 static struct sk_buff *hinic3_fetch_rx_buffer(struct hinic3_rxq *rxq,
 					      u32 pkt_len)
 {
@@ -553,7 +581,7 @@ int hinic3_configure_rxqs(struct net_device *netdev, u16 num_rq,
 int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
 {
 	struct hinic3_nic_dev *nic_dev = netdev_priv(rxq->netdev);
-	u32 sw_ci, status, pkt_len, vlan_len;
+	u32 sw_ci, status, pkt_len, vlan_len, sge_num;
 	struct hinic3_rq_cqe *rx_cqe;
 	u32 num_wqe = 0;
 	int nr_pkts = 0;
@@ -571,13 +599,21 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
 
 		vlan_len = le32_to_cpu(rx_cqe->vlan_len);
 		pkt_len = RQ_CQE_SGE_GET(vlan_len, LEN);
-		if (recv_one_pkt(rxq, rx_cqe, pkt_len, vlan_len, status))
+
+		/* The length is chosen by the device and an skb can only hold
+		 * MAX_SKB_FRAGS fragments, so a longer frame has to be dropped
+		 * rather than overrun the fragment array.
+		 */
+		sge_num = hinic3_get_sge_num(rxq, pkt_len);
+		if (unlikely(sge_num > MAX_SKB_FRAGS))
+			hinic3_discard_rx_pkt(rxq, sge_num);
+		else if (recv_one_pkt(rxq, rx_cqe, pkt_len, vlan_len, status))
 			break;
 
 		nr_pkts++;
 		num_lro = RQ_CQE_STATUS_GET(status, NUM_LRO);
 		if (num_lro)
-			num_wqe += hinic3_get_sge_num(rxq, pkt_len);
+			num_wqe += sge_num;
 
 		rx_cqe->status = 0;
 
-- 
2.43.0

Re: [PATCH net] hinic3: bound the Rx fragment count to MAX_SKB_FRAGS

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] hinic3: bound the Rx fragment count to MAX_SKB_FRAGS

[Sebastian Andrzej Siewior]

On 2026-10-10 12:09:01 [+0900], Yehyeong Lee wrote:
> hinic3_rx_poll() takes the packet length from the Rx completion
> descriptor and turns it into a fragment count:
> 
>       pkt_len = RQ_CQE_SGE_GET(vlan_len, LEN);
>       sge_num = pkt_len >> rxq->buf_len_shift ...
> 
> RQ_CQE_SGE_LEN_MASK is a 16-bit field, so pkt_len is up to 65535 and
> sge_num up to 32 with the default 2048-byte buffers.  packaging_skb()
> then calls skb_add_rx_frag() sge_num times without checking the count
> against MAX_SKB_FRAGS, so a device reporting a length above
> MAX_SKB_FRAGS * buf_len writes fragment descriptors past the end of the
> skb's shared info array.
> 
> Driving the receive path with an out-of-range length reports under
> KASAN:
> 
>       BUG: KASAN: slab-out-of-bounds in hinic3_fetch_rx_buffer [hinic3]
>       Write of size 8 ...
>       The buggy address belongs to the object ...
>        which belongs to the cache skbuff_small_head of size 704
> 
> The length is chosen by the device and an skb cannot hold more than
> MAX_SKB_FRAGS fragments, so drop a frame that needs more instead of
> overrunning the array.  The buffers it claimed are already posted on the
> ring, so return them to the page pool and advance the consumer index
> over them to keep it in step with the device.
> 
> Fixes: 17fcb3dc12bb ("hinic3: module initialization and tx/rx logic")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

How do you run into this? With the default 17 frags and 2048 buffers
your packet is larger than 17 * 2048 = 34KiB. Is this with a large MTU
or does the HW merge packets on its own? Ideally you would like to avoid
this from happening. The user gets just a dropped packet with no clue
what is wrong.

Sebastian

Re: [PATCH net] hinic3: bound the Rx fragment count to MAX_SKB_FRAGS

[Yehyeong Lee]

Thanks for the review.

A well-behaved device can't reach it. rx_buf_len is fixed at 2048
regardless of MTU, so the frag count is ceil(pkt_len / 2048): a max-MTU
frame (9600) is 5 frags, and LRO -- off by default, capped at 32K when
enabled -- is at most 16, both below the 17 the array holds. Only the
16-bit length field in the completion descriptor (RQ_CQE_SGE_LEN_MASK)
lets a device claim up to 65535 bytes, i.e. 32 frags. So it takes a
device reporting a bogus length, i.e. broken or malicious firmware,
which is why I treated it as a robustness fix -- I should have said so
in the changelog.

I found it by code inspection, not in the field, and I don't have a
hinic3 card, so it's untested on real hardware; I reproduced it by
injecting an out-of-range length to get the KASAN splat in the
changelog. The netdev bot asked for these same two things, so this
answers it as well.

You're right that the bare drop is unhelpful. It only surfaces as
rx_dropped via ndo_get_stats64, not in ethtool -S -- hinic3 has no
get_ethtool_stats, and the stats series in net-next doesn't export
dropped either. I'm happy to add a net_warn_ratelimited() on the
discard path in v2; it only fires for a misbehaving device, so it won't
spam a healthy system. Let me know if you'd like that.

For reference, eea does the same thing -- it drops once nr_frags
reaches MAX_SKB_FRAGS.

Thanks,
Yehyeong