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]

Date:   Mon, 4 Jul 2022 17:40:13 +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> :
> 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.

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

$ 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);


-- 
Ueimor