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

[Justin Lai <justinlai0215@realtek.com>]

Jakub Kicinski <kuba@kernel.org> 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.
> ---
>     rtase: Workaround for TX hang caused by short UDP packets entering
> hardware PTP parsing
> 
> This commit attempts to work around a hardware TX hang caused by
> out-of-bounds
> reads during PTP parsing. It introduces logic to pad short UDP packets to a
> minimum transport data length before transmission.
> 
> > diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> > index 255667775f0e8..944ffe71b2d07 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)
> > +{
> > +     struct ipv6hdr *i6h, _i6h;
> > +     struct iphdr *ih, _ih;
> > +     __be16 proto;
> > +     u32 no;
> > +
> > +     if (!rtase_get_l3_proto(skb, &proto, &no))
> > +             return false;
> > +
> > +     switch (proto) {
> > +     case htons(ETH_P_IP):
> > +             ih = skb_header_pointer(skb, no, sizeof(_ih), &_ih);
> > +             if (!ih)
> > +                     return false;
> 
> [Severity: High]
> Does this skip the padding workaround if the IP header itself is truncated?
> 
> If the packet is shorter than the IP header (e.g., less than 34 bytes),
> skb_header_pointer() will return false. This causes rtase_skb_pad() to
> return true, allowing the packet to skip padding. Could this small unpadded
> packet still trigger the hardware parser and cause the TX hang?
> 

Thanks for pointing this out.

A truncated IPv4 header can also trigger the same hardware TX hang
issue.

We'll update the workaround to handle this case as well.
We'll also update the commit message to document this hardware
limitation.

> [ ... ]
> > +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;
> > +
> > +     if (udp_offset > skb->len)
> > +             return false;
> > +
> > +     trans_data_len = skb->len - udp_offset;
> > +     if (trans_data_len < offsetof(struct udphdr, len) ||
> > +         trans_data_len >= RTASE_MIN_PAD_LEN)
> > +             return true;
> 
> [Severity: High]
> Can an attacker bypass the workaround if the transport payload is very small?
> 
> By returning true when trans_data_len is less than
> offsetof(struct udphdr, len) (which is 4 bytes), the packet is passed to
> the hardware unpadded.
> 
> If an attacker sends an IPv4 non-initial fragment with a very small payload,
> the hardware parser might identify the fragment as UDP and attempt to read
> the
> destination port at offset 2 from the payload. If this out-of-bounds read
> hits residual uninitialized memory in the TX FIFO containing PTP port values
> (319 or 320), the hardware could incorrectly classify it as a PTP packet,
> continue parsing out of bounds, and trigger the TX hang.
> 

Thanks for the review.

The hardware fetches packet data according to the packet length
specified in the TX descriptor, so it does not fetch residual data
beyond the end of the packet during DMA.

The affected out-of-bounds access occurs after the packet has already
matched the PTP classification criteria (UDP destination port 319/320)
and entered the hardware PTP parsing path.

If the transport data is shorter than the destination port field
itself, the packet cannot match the affected PTP classification path,
so padding is not required for this case.

> > +
> > +     dest = skb_header_pointer(skb,
> --
> pw-bot: cr