[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.