[PATCH net v2] net: wwan: t7xx: check skb_clone in control TX

[PATCH net v2] net: wwan: t7xx: check skb_clone in control TX

[Ruoyu Wang]

t7xx_port_ctrl_tx() clones each skb fragment before passing it to the
port transmit path. The clone is used immediately to set cloned->len, so
an skb_clone() failure results in a NULL pointer dereference.

Return -ENOMEM if skb_clone() fails.

Fixes: 36bd28c1cb0d ("wwan: core: Support slicing in port TX flow of WWAN subsystem")
Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
Changes in v2:
- Add Fixes tag.
- Return -ENOMEM consistently on skb_clone() failure.

 drivers/net/wwan/t7xx/t7xx_port_wwan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
index 7fc569565..01844d067 100644
--- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
+++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
@@ -106,6 +106,8 @@ static int t7xx_port_ctrl_tx(struct t7xx_port *port, struct sk_buff *skb)
 
 	while (cur) {
 		cloned = skb_clone(cur, GFP_KERNEL);
+		if (!cloned)
+			return -ENOMEM;
 		cloned->len = skb_headlen(cur);
 		ret = t7xx_port_send_skb(port, cloned, 0, 0);
 		if (ret) {
-- 
2.51.0

Re: [PATCH net v2] net: wwan: t7xx: check skb_clone in control TX

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: wwan: t7xx: check skb_clone in control TX

This patch checks the return value of skb_clone() in t7xx_port_ctrl_tx()
and returns -ENOMEM if it fails, preventing a NULL pointer dereference.

> diff --git a/drivers/net/wwan/t7xx/t7xx_port_wwan.c b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> --- a/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> +++ b/drivers/net/wwan/t7xx/t7xx_port_wwan.c
> @@ -106,6 +106,8 @@ static int t7xx_port_ctrl_tx(struct t7xx_port *port, struct sk_buff *skb)
>  
>  	while (cur) {
>  		cloned = skb_clone(cur, GFP_KERNEL);
> +		if (!cloned)
> +			return -ENOMEM;

[Severity: High]
If we return -ENOMEM directly here when skb_clone() fails on a subsequent
fragment, does this discard the count of successfully transmitted bytes
accumulated in cnt?

If t7xx_port_send_skb() previously succeeded for earlier fragments, returning
-ENOMEM to wwan_port_fops_write() will leave userspace unaware that partial
data was already consumed. Will userspace then retry writing the entire
buffer and duplicate the already-sent fragments on the WWAN stream?

>  		cloned->len = skb_headlen(cur);

Simon says: I have trimmed the remaining issues flagged in the AI-generated
            review as I believe that they are pre-existing and do not
	    warrant being considered in the context of this patch.

	    However, you may want to look over them on sashiko.dev
	    in the context of possible follow-up separate to this patch.