Re: [PATCH net] r8169: work around RTL8125 UDP hw bug

[Heiner Kallweit <hkallweit1@gmail.com>]

On 27.01.2021 21:35, Willem de Bruijn wrote:
> On Wed, Jan 27, 2021 at 3:32 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 27.01.2021 20:54, Willem de Bruijn wrote:
>>> On Wed, Jan 27, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>>
>>>> On 27.01.2021 19:07, Willem de Bruijn wrote:
>>>>> On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallweit1@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.bn@gmail.com>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@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) ?

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?