On Wed, Jan 27, 2021 at 4:34 PM Heiner Kallweit <hkallwe...@gmail.com> wrote: > > On 27.01.2021 21:35, Willem de Bruijn wrote: > > On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit <hkallwe...@gmail.com> > > wrote: > >> > >> On 27.01.2021 20:54, Willem de Bruijn wrote: > >>> On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit <hkallwe...@gmail.com> > >>> wrote: > >>>> > >>>> On 27.01.2021 19:07, Willem de Bruijn wrote: > >>>>> On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallwe...@gmail.com> > >>>>> wrote: > >>>>>> > >>>>>> It was reported that on RTL8125 network breaks under heavy UDP load, > >>>>>> e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug > >>>>>> and provided me with a test version of the r8125 driver including a > >>>>>> workaround. Tests confirmed that the workaround fixes the issue. > >>>>>> I modified the original version of the workaround to meet mainline > >>>>>> code style. > >>>>>> > >>>>>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=209839 > >>>>>> > >>>>>> Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125") > >>>>>> Tested-by: xplo <xplo...@gmail.com> > >>>>>> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com> > >>>>>> --- > >>>>>> drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++--- > >>>>>> 1 file changed, 58 insertions(+), 6 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c > >>>>>> b/drivers/net/ethernet/realtek/r8169_main.c > >>>>>> index fb67d8f79..90052033b 100644 > >>>>>> --- a/drivers/net/ethernet/realtek/r8169_main.c > >>>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c > >>>>>> @@ -28,6 +28,7 @@ > >>>>>> #include <linux/bitfield.h> > >>>>>> #include <linux/prefetch.h> > >>>>>> #include <linux/ipv6.h> > >>>>>> +#include <linux/ptp_classify.h> > >>>>>> #include <asm/unaligned.h> > >>>>>> #include <net/ip6_checksum.h> > >>>>>> > >>>>>> @@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct > >>>>>> rtl8169_private *tp, struct sk_buff *skb, > >>>>>> return -EIO; > >>>>>> } > >>>>>> > >>>>>> -static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp) > >>>>>> +static bool rtl_skb_is_udp(struct sk_buff *skb) > >>>>>> { > >>>>>> + switch (vlan_get_protocol(skb)) { > >>>>>> + case htons(ETH_P_IP): > >>>>>> + return ip_hdr(skb)->protocol == IPPROTO_UDP; > >>>>>> + case htons(ETH_P_IPV6): > >>>>>> + return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP; > >>>> > >>>> The workaround was provided by Realtek, I just modified it to match > >>>> mainline code style. For your reference I add the original version below. > >>>> I don't know where the magic numbers come from, Realtek releases > >>>> neither data sheets nor errata information. > >>> > >>> Okay. I don't know what is customary for this process. > >>> > >>> But I would address the possible out of bounds read by trusting ip > >>> header integrity in rtl_skb_is_udp. > >>> > >> I don't know tun/virtio et al good enough to judge which header elements > >> may be trustworthy and which may be not. What should be checked where? > > > > It requires treating the transmit path similar to the receive path: > > assume malicious or otherwise faulty packets. So do not trust that a > > protocol of ETH_P_IPV6 implies a packet with 40B of space to hold a > > full ipv6 header. That is the extent of it, really. > > > OK, so what can I do? Check for > skb_tail_pointer(skb) - skb_network_header(skb) >= sizeof(struct ipv6hdr) ?
It is quite rare for device drivers to access protocol header fields (and a grep points to lots of receive side operations), so I don't have a good driver example. But qdisc_pkt_len_init in net/core/dev.c shows a good approach for this robust access in the transmit path: using skb_header_pointer. > On a side note: Why is IP6_HLEN defined in ptp_classify.h and not in any > IPv6 header file? Does no IPv6 code need such a constant? It is customary to use sizeof(struct ipv6hdr)
