Re: [PATCH net v3] rtase: Workaround for TX hang caused by short UDP packets entering hardware PTP parsing

[Simon Horman <horms@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.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
rtase: Workaround for TX hang caused by short UDP packets entering hardware PTP parsing

This patch adds a workaround for a hardware TX hang that occurs when
short UDP packets matching destination ports 319/320 enter a PTP parsing
path that reads beyond the packet end. The workaround pads affected
packets so the transport data reaches RTASE_MIN_PAD_LEN before
transmission.

> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> index 55105d34bc79..4c295a39c7a0 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c

[ ... ]

> +static bool rtase_get_udp_offset(struct sk_buff *skb, u32 *udp_offset)
> +{
> +	int no = skb_network_offset(skb);
> +	struct ipv6hdr *i6h, _i6h;
> +	struct iphdr *ih, _ih;
> +
> +	switch (vlan_get_protocol(skb)) {
> +	case htons(ETH_P_IP):
> +		ih = skb_header_pointer(skb, no, sizeof(_ih), &_ih);
> +		if (!ih)
> +			return false;
> +
> +		if (ih->ihl < 5)
> +			return false;
> +
> +		if (ih->protocol != IPPROTO_UDP)
> +			return false;
> +
> +		*udp_offset = no + ih->ihl * 4;

[Low]
Should ih->ihl also be checked against the actual data present in the
skb? With ihl up to 15 (60 bytes), if the skb only contains the basic
20-byte IP header, the computed udp_offset can exceed skb->len.

Downstream in rtase_skb_pad():

	trans_data_len = skb->len - udp_offset;

trans_data_len is u32, so when udp_offset > skb->len this underflows to
a very large value, which trivially satisfies:

	trans_data_len >= RTASE_MIN_PAD_LEN

and rtase_skb_pad() silently skips the padding. A malformed packet from
an AF_PACKET sender with CAP_NET_RAW could craft a mismatched ihl and
bypass the workaround. Would clamping or validating udp_offset against
skb->len before the subtraction be appropriate?

> +
> +		return true;
> +	case htons(ETH_P_IPV6):
> +		i6h = skb_header_pointer(skb, no, sizeof(_i6h), &_i6h);
> +		if (!i6h)
> +			return false;
> +
> +		if (i6h->nexthdr != IPPROTO_UDP)
> +			return false;
> +
> +		*udp_offset = no + sizeof(*i6h);
> +
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool rtase_skb_pad(struct sk_buff *skb)
> +{
> +	__be16 *dest, _dest;
> +	u32 trans_data_len;
> +	u32 udp_offset;
> +	u16 dest_port;
> +	u32 pad_len;
> +
> +	if (!rtase_get_udp_offset(skb, &udp_offset))
> +		return true;
> +
> +	trans_data_len = skb->len - udp_offset;
> +	if (trans_data_len < offsetof(struct udphdr, len) ||
> +	    trans_data_len >= RTASE_MIN_PAD_LEN)
> +		return true;

[Low]
When trans_data_len is less than offsetof(struct udphdr, len) (i.e., the
transport region is under 4 bytes), this returns true and skips the
padding entirely.

The commit message states the hardware "may access data beyond the end
of the packet" during PTP parsing. If the hardware also reads out of
bounds while classifying the packet, an IPv4 last fragment with 1-3
trailing bytes could still be misclassified as PTP based on whatever
garbage memory the hardware happens to read, and the workaround would
be bypassed.

The implicit assumption here seems to be "if the driver cannot read the
dest port from the skb, the hardware cannot classify it either", which
appears to contradict the premise that the hardware reads beyond the
packet boundary. Should these very short transport-data cases also be
padded to be safe?

> +
> +	dest = skb_header_pointer(skb,
> +				  udp_offset + offsetof(struct udphdr, dest),
> +				  sizeof(_dest), &_dest);

[ ... ]