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: <YsI6bEFtM+2uK492@electric-eye.fr.zoreil.com>
Date:   Mon, 4 Jul 2022 02:55:08 +0200
From:   Francois Romieu <romieu@...zoreil.com>
To:     Heiner Kallweit <hkallweit1@...il.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

Heiner Kallweit <hkallweit1@...il.com> :
> 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>

/me wonders...

- bz216157 experiences a (2nd) warning because the rtl8169_tso_csum_v2
  ARP path shares the factored read of the (unset) transport offset
  but said ARP path does not use the transport offset.
  -> ok, the warning is mostly harmless. 

- rtl8169_tso_csum_v2 non-ARP paths own WARN_ON_ONCE will always
  complain before Eric's transport specific warning triggers.
  -> ok, the warning is redundant.

- rtl8169_features_check

> 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
[...]
> @@ -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;
>  	}

Neither skb_is_gso nor CHECKSUM_PARTIAL implies a transport header so the
warning may still trigger, right ?

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.

-- 
Ueimor