netdev - Re: [PATCH net] r8169: fix accessing unset transport header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0c3da9d8-20fb-81ef-23a6-ab88921cf0d2@googlemail.com>
Date:   Mon, 4 Jul 2022 21:10:56 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Francois Romieu <romieu@...zoreil.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Erhard F." <erhard_f@...lbox.org>
Subject: Re: [PATCH net] r8169: fix accessing unset transport header

On 04.07.2022 17:40, Francois Romieu wrote:
> Heiner Kallweit <hkallweit1@...il.com> :
>> On 04.07.2022 02:55, Francois Romieu wrote:
>>> Heiner Kallweit <hkallweit1@...il.com> :
>>>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>>> [...]
>>>> @@ -4420,7 +4418,7 @@ static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
>>>>  		if (rtl_quirk_packet_padto(tp, skb))
>>>>  			features &= ~NETIF_F_CSUM_MASK;
>>>>  
>>>> -		if (transport_offset > TCPHO_MAX &&
>>>> +		if (skb_transport_offset(skb) > TCPHO_MAX &&
>>>>  		    rtl_chip_supports_csum_v2(tp))
>>>>  			features &= ~NETIF_F_CSUM_MASK;
>>>>  	}
>>>
>>> Neither skb_is_gso nor CHECKSUM_PARTIAL implies a transport header so the
>>> warning may still trigger, right ?
>>
>> I'm not an expert here, and due to missing chip documentation I can't say
>> whether the chip could handle hw csumming correctly w/o transport header.
>> I'd see whether we get more reports of this warning. If yes, then maybe
>> we should use skb_transport_header_was_set() explicitly and disable
>> hw csumming if there's no transport header.
> 
> (some sleep later)
> 
> I had forgotten the NETIF_F_* stuff in the r8169 driver. :o/
> 
> So, yes, ignore this point.
> 
>>> Btw it's a bit unexpected to see a "Fixes" tag related to a RTL8125 bug as
>>> well as a "Tested-by" by the bugzilla submitter when the dmesg included in
>>> bz216157 exibits a RTL8168e/8111e.
>>>
>> The Fixes tag refers to the latest change to the affected code, therefore
>> it comes a little unexpected, right.
> 
> ?
> 
> 8d520b4de3ed does not change the affected code.
> 

This chunk of 8d520b4de3ed 

@@ -4128,9 +4183,10 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 
 		opts[1] |= transport_offset << TCPHO_SHIFT;
 	} else {
-		if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
-			/* eth_skb_pad would free the skb on error */
-			return !__skb_put_padto(skb, ETH_ZLEN, false);
+		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
+
+		/* skb_padto would free the skb on error */
+		return !__skb_put_padto(skb, padto, false);
 	}
 
 	return true;

changes the context for this part of the patch. Therefore the patch wouldn't
apply cleanly.


@@ -4235,7 +4234,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
 		else
 			WARN_ON_ONCE(1);
 
-		opts[1] |= transport_offset << TCPHO_SHIFT;
+		opts[1] |= skb_transport_offset(skb) << TCPHO_SHIFT;
 	} else {
 		unsigned int padto = rtl_quirk_packet_padto(tp, skb);
 


> Eric's unset transport offset detection debug code would have produced the
> same output with the parent of the "Fixes" commit id:
> 

I know, but due to the fact that the warnings are harmless and the new check
doesn't exist in earlier versions, I think we can omit these kernel versions.

> $ git cat-file -p 8d520b4de3ed^:drivers/net/ethernet/realtek/r8169_main.c | grep -A4 -B1 -E 'rtl8169_features_check'
> 
> static netdev_features_t rtl8169_features_check(struct sk_buff *skb,
> 						struct net_device *dev,
> 						netdev_features_t features)
> {
> 	int transport_offset = skb_transport_offset(skb);
> --
> 	.ndo_start_xmit		= rtl8169_start_xmit,
> 	.ndo_features_check	= rtl8169_features_check,
> 	.ndo_tx_timeout		= rtl8169_tx_timeout,
> 	.ndo_validate_addr	= eth_validate_addr,
> 	.ndo_change_mtu		= rtl8169_change_mtu,
> 	.ndo_fix_features	= rtl8169_fix_features,
> 
> 
> -> 8d520b4de3ed does not modify the first
> 'int transport_offset = skb_transport_offset(skb);' statement and neither
> does it modify the code path to rtl8169_features_check 
> 
> 8d520b4de3ed actually removes some logical path towards rtl8169_tso_csum_v2
> but it does not change (nor does it break) the relevant code:
> 
> $ git cat-file -p 8d520b4de3ed^:drivers/net/ethernet/realtek/r8169_main.c | grep -A3 -B1 -E 'bool rtl8169_tso_csum_v2'
> 
> static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
> 				struct sk_buff *skb, u32 *opts)
> {
> 	u32 transport_offset = (u32)skb_transport_offset(skb);
> 
>