Re: [PATCH net-next v17 11/13] ax88179_178a: Update driver name and information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: skb data corruption in ax88179a_tx_fixup due to missing skb_cloned check
- [High] pm: leaked pm_mode state in ax88179a_suspend error path
--

commit beecb9dc824b9046a2ddbab47f74bb933e48f1e8
Author: Birger Koblitz <mail@birger-koblitz.de>

    ax88179_178a: Update driver name and information

    Add additionally supported devices to Kconfig description, add further
    requirements such as PHYLINK. Update driver name in usb_driver and
    update MODULE_DESCRIPTION to include the additionally supported chips.

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> new file mode 100644
> index 0000000000000..929fa51280e84
> --- /dev/null
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -0,0 +1,1284 @@

[ ... ]

> +static int ax88179a_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct usbnet *dev = usb_get_intfdata(intf);
> +	struct ax88179_data *priv;
> +	u8 tmp8 = 0;
> +	u16 tmp16;
> +	int ret;
> +
> +	priv = dev->driver_priv;
> +	ax88179_set_pm_mode(dev, true);
> +
> +	if (netif_running(dev->net)) {
> +		rtnl_lock();
> +		phylink_suspend(priv->phylink, !!priv->wolopts);
> +		rtnl_unlock();
> +	}
> +
> +	ret = usbnet_suspend(intf, message);
> +	if (ret) {
> +		rtnl_lock();
> +		phylink_resume(priv->phylink);
> +		rtnl_unlock();
> +		return ret;
                ^^^^

[Severity: High]
Could this early return bypass ax88179_set_pm_mode(dev, false) and leave the
driver's internal PM state permanently corrupted?

If in_pm remains true, this could cause subsequent control commands to
bypass runtime PM locks, leading to aggressive auto-suspend and eventual
USB timeouts.

> +	}

[ ... ]

> +static struct sk_buff *ax88179a_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
> +{
> +	u64 tx_desc = skb->len & AX179A_TX_DESC_LEN_MASK;
> +	int frame_size = dev->maxpacket;
> +	struct sk_buff *ax_skb;
> +	u64 *tx_desc_ptr;
> +	int padding_size;
> +	int headroom;
> +	int tailroom;
> +	u16 tci = 0;
> +
> +	/* TSO MSS */
> +	tx_desc |= FIELD_PREP(AX179A_TX_DESC_MSS, (u64)skb_shinfo(skb)->gso_size);
> +
> +	headroom = (skb->len + sizeof(tx_desc)) % 8;
> +	padding_size = headroom ? 8 - headroom : 0;
> +
> +	if (((skb->len + sizeof(tx_desc) + padding_size) % frame_size) == 0) {
> +		padding_size += 8;
> +		tx_desc |= AX179A_TX_DESC_DROP_PADD;
> +	}
> +
> +	if ((dev->net->features & NETIF_F_HW_VLAN_CTAG_TX) && (vlan_get_tag(skb, &tci) >= 0)) {
> +		tx_desc |= AX179A_TX_DESC_VLAN;
> +		tx_desc |= FIELD_PREP(AX179A_TX_DESC_VLAN_ID, (u64)tci);
> +	}
> +
> +	if (!dev->can_dma_sg && (dev->net->features & NETIF_F_SG) && skb_linearize(skb)) {
> +		dev_kfree_skb_any(skb);
> +		return NULL;
> +	}
> +
> +	headroom = skb_headroom(skb);
> +	tailroom = skb_tailroom(skb);
> +
> +	if (!(headroom >= sizeof(tx_desc) && tailroom >= padding_size)) {
> +		ax_skb = skb_copy_expand(skb, sizeof(tx_desc), padding_size, flags);
> +		dev_kfree_skb_any(skb);
> +		skb = ax_skb;
> +		if (!skb)
> +			return NULL;
> +	}
> +	if (padding_size != 0)
> +		skb_put_zero(skb, padding_size);
> +	/* Copy TX header */
> +	tx_desc_ptr = skb_push(skb, sizeof(tx_desc));
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Are we writing into the SKB headroom here without checking if the buffer is
shared?

If a cloned SKB (such as during packet capture, TCP retransmits, or
broadcast) happens to have sufficient headroom and tailroom, this skips
skb_copy_expand and directly modifies the shared buffer.

This would corrupt data for all other clones holding a reference. Should
this call skb_cloned() or unshare the SKB buffer first?

> +	put_unaligned_le64(tx_desc, tx_desc_ptr);
> +
> +	usbnet_set_skb_tx_stats(skb, 1, 0);
> +
> +	return skb;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-ax88179a-v17-0-252814fe35ea@birger-koblitz.de?part=11