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