Re: [PATCH net-next v2 08/10] net: dsa: microchip: add KSZ8463 tail tag handling

[Bastien Curutchet]

Hi Simon,

On 7/20/26 4:07 PM, Simon Horman 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.
> https://sashiko.dev/#/patchset/20260713-ksz-new-ptp-v2-1-7209c05058ef@xxxxxxxxxxx?part=8
> ---
> --- Patch [8]: [PATCH net-next v2 08/10] net: dsa: microchip: add KSZ8463 tail tag handling ---
> commit bc95662a9488a7827d30f83890008a818f0a5228
> Author: Bastien Curutchet (Schneider Electric) <bastien.curutchet@xxxxxxxxxxx>
>
> net: dsa: microchip: add KSZ8463 tail tag handling
>
> KSZ8463 uses the KSZ9893 DSA TAG driver, but uses reserved fields in the PTP
> header for timestamps instead of the tail tag. This commit adds a
> KSZ8463-specific DSA_TAG driver to handle KSZ8463 timestamps.
>
> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index f8b40437c5fa4..477f2997c1a34 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c
>
> [ ... ]
>
> > +static struct sk_buff *ksz8463_rcv(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	unsigned int len = KSZ_EGRESS_TAG_LEN;
> > +	struct ptp_header *ptp_hdr;
> > +	unsigned int ptp_class;
> > +	unsigned int port;
> > +	ktime_t ts;
> > +	u8 *tag;
> > +
> > +	if (skb_linearize(skb)) {
> > +		kfree_skb(skb);
> > +		return NULL;
> > +	}
> > +
> > +	/* Tag decoding */
> > +	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> > +	port = tag[0] & KSZ8463_TAIL_TAG_EG_PORT_M;
> > +
> > +	__skb_push(skb, ETH_HLEN);
> > +	ptp_class = ptp_classify_raw(skb);
> > +	__skb_pull(skb, ETH_HLEN);
> > +	if (ptp_class == PTP_CLASS_NONE)
> > +		goto common_rcv;
> > +
> > +	ptp_hdr = ptp_parse_header(skb, ptp_class);
> > +	if (ptp_hdr) {
> > +		ts = ksz_decode_tstamp(get_unaligned_be32(&ptp_hdr->reserved2));
> > +		KSZ_SKB_CB(skb)->tstamp = ts;
> > +		ptp_hdr->reserved2 = 0;
> > +		skb->ip_summed = CHECKSUM_NONE;
>
> [Severity: High]
> Does this unconditionally modify the payload of all PTP packets?
>
> If a PTPv1 packet is received, ptp_classify_raw() identifies it as PTP, but
> the PTPv1 header structure is different. Blindly casting and zeroing
> ptp_hdr->reserved2 aliases bytes 16-19, which corrupts the subdomain string.
> Should this check that ptp_class includes PTP_CLASS_V2?

I don't find in the code a different PTP header for the PTP_CLASS_V1. So 
my guess is that the reserved2 field is also reserved for V1, so setting 
it to zero shouldn't matter IMO.

> Additionally, altering the UDP payload without updating the UDP checksum and
> setting skb->ip_summed = CHECKSUM_NONE forces the network stack to verify the
> now-invalid checksum, which causes the packet to be dropped.

I think I misunderstood Sashiko comment on the previous series and 
shouldn't have added the skb->ip_summed = CHECKSUM_NONE line in this one.

The switch uses the reserved2 field to convey the frame timestamp. 
Datasheet doesn't say whether or not it updates the UDP checksum when 
doing so. What I'm sure of is that packets aren't dropped as the PTP 
synchronization is successful, so I think the UDP checksum isn't touched 
by the switch.
I'm not sure to understand how the skb->ip_summed field is supposed to 
be used so any guideline here would be welcome.

> > +	}
>
> [Severity: High]
> Can this leak uninitialized memory to userspace if a truncated PTP packet is
> received?
>
> If ptp_parse_header() returns NULL, KSZ_SKB_CB(skb)->tstamp is left
> uninitialized. Later, when the DSA core calls dsa_skb_defer_rx_timestamp(),
> this uninitialized value is read and exposed to userspace:
Ack, I'll initialize KSZ_SKB_CB(skb)->tstamp to 0 before.


Best regards,
Bastien