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 for Android: free password hash cracker in your pocket

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

Message-ID: <5a11a710-2de6-a6ed-b4f6-0c6016cf05c1@gmail.com>
Date:   Tue, 5 Jul 2022 21:11:45 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>
Cc:     "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 05.07.2022 14:46, Paolo Abeni wrote:
> On Mon, 2022-07-04 at 00:12 +0200, Heiner Kallweit wrote:
>> 66e4c8d95008 ("net: warn if transport header was not set") added
>> a check that triggers a warning in r8169, see [0].
>>
>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=216157
>>
>> Fixes: 8d520b4de3ed ("r8169: work around RTL8125 UDP hw bug")
>> Reported-by: Erhard F. <erhard_f@...lbox.org>
>> Tested-by: Erhard F. <erhard_f@...lbox.org>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> 
> The patch LGTM, but I think you could mention in the commit message
> that the bug was [likely] introduced with commit bdfa4ed68187 ("r8169:
> use Giant Send"), but this change applies only on top of the commit
> specified by the fixes tag - just to help stable teams.
> 
Right, I'll submit a v2 with more details in the commit message.

> Thanks!
> 
> Paolo
> 
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index 3098d6672..1b7fdb4f0 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -4190,7 +4190,6 @@ static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)
>>  static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>>  				struct sk_buff *skb, u32 *opts)
>>  {
>> -	u32 transport_offset = (u32)skb_transport_offset(skb);
>>  	struct skb_shared_info *shinfo = skb_shinfo(skb);
>>  	u32 mss = shinfo->gso_size;
>>  
>> @@ -4207,7 +4206,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
>>  			WARN_ON_ONCE(1);
>>  		}
>>  
>> -		opts[0] |= transport_offset << GTTCPHO_SHIFT;
>> +		opts[0] |= skb_transport_offset(skb) << GTTCPHO_SHIFT;
>>  		opts[1] |= mss << TD1_MSS_SHIFT;
>>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>>  		u8 ip_protocol;
>> @@ -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);
>>  
>> @@ -4402,14 +4401,13 @@ 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);
>>  	struct rtl8169_private *tp = netdev_priv(dev);
>>  
>>  	if (skb_is_gso(skb)) {
>>  		if (tp->mac_version == RTL_GIGA_MAC_VER_34)
>>  			features = rtl8168evl_fix_tso(skb, features);
>>  
>> -		if (transport_offset > GTTCPHO_MAX &&
>> +		if (skb_transport_offset(skb) > GTTCPHO_MAX &&
>>  		    rtl_chip_supports_csum_v2(tp))
>>  			features &= ~NETIF_F_ALL_TSO;
>>  	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> @@ -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;
>>  	}
>