[PATCH] net: wwan: t7xx: fix potential skb->frags overflow in RX path

[PATCH] net: wwan: t7xx: fix potential skb->frags overflow in RX path

[Kery Qi]

When receiving data in the DPMAIF RX path,
the t7xx_dpmaif_set_frag_to_skb() function adds
page fragments to an skb without checking if the number of
fragments has exceeded MAX_SKB_FRAGS. This could lead to a buffer overflow
in skb_shinfo(skb)->frags[] array, corrupting adjacent memory and
potentially causing kernel crashes or other undefined behavior.

This issue was identified through static code analysis by comparing with a
similar vulnerability fixed in the mt76 driver commit b102f0c522cf ("mt76:
fix array overflow on receiving too many fragments for a packet").

The vulnerability could be triggered if the modem firmware sends packets
with excessive fragments. While under normal protocol conditions (MTU 3080
bytes, BAT buffer 3584 bytes),
a single packet should not require additional
fragments, the kernel should not blindly trust firmware behavior.
Malicious, buggy, or compromised firmware could potentially craft packets
with more fragments than the kernel expects.

Fix this by adding a bounds check before calling skb_add_rx_frag()
to ensure nr_frags does not exceed MAX_SKB_FRAGS.

Fixes: d642b012df70a ("net: wwan: t7xx: Add data path interface")
Signed-off-by: Kery Qi <qikeyu2017@gmail.com>
---
 drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
index b76bea6ab2d7..b041e6f48732 100644
--- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
+++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
@@ -395,6 +395,7 @@ static int t7xx_dpmaif_set_frag_to_skb(const struct dpmaif_rx_queue *rxq,
 				       struct sk_buff *skb)
 {
 	unsigned long long data_bus_addr, data_base_addr;
+	struct skb_shared_info *shinfo = skb_shinfo(skb);
 	struct device *dev = rxq->dpmaif_ctrl->dev;
 	struct dpmaif_bat_page *page_info;
 	unsigned int data_len;
@@ -407,13 +408,16 @@ static int t7xx_dpmaif_set_frag_to_skb(const struct dpmaif_rx_queue *rxq,
 	if (!page_info->page)
 		return -EINVAL;
 
+	if (shinfo->nr_frags >= MAX_SKB_FRAGS)
+		return -EINVAL;
+
 	data_bus_addr = le32_to_cpu(pkt_info->pd.data_addr_h);
 	data_bus_addr = (data_bus_addr << 32) + le32_to_cpu(pkt_info->pd.data_addr_l);
 	data_base_addr = page_info->data_bus_addr;
 	data_offset = data_bus_addr - data_base_addr;
 	data_offset += page_info->offset;
 	data_len = FIELD_GET(PD_PIT_DATA_LEN, le32_to_cpu(pkt_info->header));
-	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page_info->page,
+	skb_add_rx_frag(skb, shinfo->nr_frags, page_info->page,
 			data_offset, data_len, page_info->data_len);
 
 	page_info->page = NULL;
-- 
2.34.1

Re: net: wwan: t7xx: fix potential skb->frags overflow in RX path

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: wwan: t7xx: fix potential skb->frags overflow in RX path

This patch adds a bounds check to prevent skb->frags[] array overflow
when the modem firmware sends packets with excessive fragments. The fix
adds a check for nr_frags >= MAX_SKB_FRAGS before calling skb_add_rx_frag().

> diff --git a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> index b76bea6ab2d7..b041e6f48732 100644
> --- a/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> +++ b/drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c
> @@ -395,6 +395,7 @@ static int t7xx_dpmaif_set_frag_to_skb(const struct dpmaif_rx_queue *rxq,
>  				       struct sk_buff *skb)
>  {
>  	unsigned long long data_bus_addr, data_base_addr;
> +	struct skb_shared_info *shinfo = skb_shinfo(skb);
>  	struct device *dev = rxq->dpmaif_ctrl->dev;
>  	struct dpmaif_bat_page *page_info;
>  	unsigned int data_len;
> @@ -407,13 +408,16 @@ static int t7xx_dpmaif_set_frag_to_skb(const struct dpmaif_rx_queue *rxq,
>  	if (!page_info->page)
>  		return -EINVAL;
>
> +	if (shinfo->nr_frags >= MAX_SKB_FRAGS)
> +		return -EINVAL;

When this new error path is taken, it appears that page_info->page has
already had its DMA mapping removed via dma_unmap_page() earlier in the
function, but the page itself is not freed (put_page is not called) and
page_info->page is not set to NULL.

Could this cause a page leak? On this error path:
- dma_unmap_page() has already been called
- page_info->page still holds a reference to the page
- t7xx_dpmaif_set_bat_mask() is not called in the caller t7xx_dpmaif_get_frag()

When the device is eventually torn down, t7xx_dpmaif_bat_free() will
iterate all entries and call t7xx_unmap_bat_page(), which will call
dma_unmap_page() again on the same (now stale) data_bus_addr. Is there a
concern about calling dma_unmap_page() twice for the same mapping?

It looks like the page cleanup on this error path should either:
- Move the new bounds check before dma_unmap_page(), or
- Add put_page() and set page_info->page = NULL on the error path

> +
>  	data_bus_addr = le32_to_cpu(pkt_info->pd.data_addr_h);

[ ... ]
-- 
pw-bot: cr