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

[Bastien Curutchet]

Hi

On 7/31/26 4:23 AM, Jakub Kicinski wrote:
> 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@xxxxxxxxxxx>
>
> 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.

I think it's safe to assume that ksz8795 will never use a clone as it 
doesn't support PTP, am I wrong here ?

> 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.

Best regards,
Bastien