Re: [PATCH net-next v3 07/10] net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [7]: [PATCH net-next v3 07/10] net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations ---
commit 801d0cdc8ef18f0fbd60b1d666b46b36437fd06e
Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>

net: dsa: tag_ksz: share code for KSZ8795 and KSZ9893 xmit operations

This factors the duplicated tail-tag encoding of ksz8795_xmit() and
ksz9893_xmit() into a new ksz_common_xmit() helper parameterized by
do_tstamp, prio and override_mask.  As a side effect ksz8795_xmit() now
returns through ksz_defer_xmit() instead of returning the skb directly.

> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index f58ce0f0e9e42..f8b40437c5fa4 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -218,6 +218,37 @@ static struct sk_buff *ksz_defer_xmit(struct dsa_port *dp, struct sk_buff *skb)
>  	return NULL;
>  }
>  
> +static struct sk_buff *ksz_common_xmit(struct sk_buff *skb,
> +				       struct net_device *dev,
> +				       bool do_tstamp,
> +				       u8 prio,
> +				       u8 override_mask)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(dev);
> +	struct ethhdr *hdr;
> +	u8 *tag;
> +
> +	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +
> +	/* Tag encoding */
> +	if (do_tstamp)
> +		ksz_xmit_timestamp(dp, skb);
> +
> +	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
> +	hdr = skb_eth_hdr(skb);
> +
> +	*tag = dsa_xmit_port_mask(skb, dev);
> +	*tag |= prio;
> +
> +	if (is_link_local_ether_addr(hdr->h_dest))
> +		*tag |= override_mask;
> +
> +	return ksz_defer_xmit(dp, skb);
> +}
> +

[Severity: Medium]
Can this dereference NULL for the KSZ8795 tagger?  ksz_defer_xmit() reads
ds->tagger_data and the tagger private data before using them:

net/dsa/tag_ksz.c:ksz_defer_xmit() {
	struct ksz_tagger_data *tagger_data = ksz_tagger_data(dp->ds);
	struct ksz_tagger_private *priv = ksz_tagger_private(dp->ds);
	...
	if (!clone)
		return skb;  /* no deferred xmit for this packet */

	xmit_work_fn = tagger_data->xmit_work_fn;
	xmit_worker = priv->xmit_worker;
	...
}

Both pointers are only populated by ksz_connect(), and ksz8795_netdev_ops
does not install it:

static const struct dsa_device_ops ksz8795_netdev_ops = {
	.name	= KSZ8795_NAME,
	.proto	= DSA_TAG_PROTO_KSZ8795,
	.xmit	= ksz8795_xmit,
	.rcv	= ksz8795_rcv,
	.needed_tailroom = KSZ_INGRESS_TAG_LEN,
};

compared with ksz9893_netdev_ops which has .connect = ksz_connect and
.disconnect = ksz_disconnect.  ksz87xx_connect_tag_protocol() and
ksz88xx_connect_tag_protocol() in drivers/net/dsa/microchip/ksz8.c also
just return 0 for DSA_TAG_PROTO_KSZ8795, so ds->tagger_data stays NULL for
these switches.

So after this change the only thing keeping ksz8795_xmit() from a NULL
dereference is KSZ_SKB_CB(skb)->clone always being NULL.

Is that invariant actually enforced anywhere?  The ksz87xx/ksz88xx
dsa_switch_ops export .port_hwtstamp_set = ksz_hwtstamp_set and
.port_txtstamp = ksz_port_txtstamp unconditionally, dsa_user_hwtstamp_set()
adds no capability gate, and ksz_set_hwtstamp_config() sets the flag before
the register write and returns the error without reverting it, with no
dev->info->ptp_capable check:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_set_hwtstamp_config() {
	...
		prt->hwts_tx_en = true;

		ret = ksz_rmw16(dev, regs[PTP_MSG_CONF1], PTP_1STEP, PTP_1STEP);
		if (ret)
			return ret;
	...
}

For these chips regs[] is ksz8795_regs, whose last initialized index is
REG_PORT_PME_CTRL (23) while PTP_MSG_CONF1 is 28, so regs[PTP_MSG_CONF1] is
an out-of-bounds read and the write can well be rejected, which is exactly
the path that leaves hwts_tx_en latched.

Once hwts_tx_en is set on such a port, ksz_port_txtstamp() attaches a clone
based on that flag alone:

drivers/net/dsa/microchip/ksz_ptp.c:ksz_port_txtstamp() {
	...
	KSZ_SKB_CB(skb)->clone = clone;
	...
}

and then dsa_user_xmit() -> ksz8795_xmit() -> ksz_common_xmit() ->
ksz_defer_xmit() gets past the "if (!clone)" early exit and loads
tagger_data->xmit_work_fn from a NULL pointer.  Before this patch
ksz8795_xmit() returned the skb directly and never touched ds->tagger_data,
so the same misconfiguration only leaked the clone.

The commit message states:

    Clones are only created by the KSZ driver when the PTP feature is
    enabled. Since KSZ8795 doesn't support PTP, returning the SKB directly
    or ksz_defer_xmit() is the same.

Could this assumption be expressed in the code, for instance by keying the
ksz_defer_xmit() call off do_tstamp, or by adding a DEBUG_NET_WARN_ON()
for a clone on a tagger without tagger_data?  Note also that
ksz8795_netdev_ops uses .needed_tailroom = KSZ_INGRESS_TAG_LEN only, so
there is no room for the 4 byte PTP tag if a future user of this helper
passes do_tstamp = true with that tailroom.

>  static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
>  				    struct net_device *dev)
>  {
> @@ -308,23 +339,7 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9477, KSZ9477_NAME);
>  
>  static struct sk_buff *ksz8795_xmit(struct sk_buff *skb, struct net_device *dev)
>  {
> -	struct ethhdr *hdr;
> -	u8 *tag;
> -
> -	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb)) {
> -		kfree_skb(skb);
> -		return NULL;
> -	}
> -
> -	/* Tag encoding */
> -	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
> -	hdr = skb_eth_hdr(skb);
> -
> -	*tag = dsa_xmit_port_mask(skb, dev);
> -	if (is_link_local_ether_addr(hdr->h_dest))
> -		*tag |= KSZ8795_TAIL_TAG_OVERRIDE;
> -
> -	return skb;
> +	return ksz_common_xmit(skb, dev, false, 0, KSZ8795_TAIL_TAG_OVERRIDE);
>  }

[ ... ]