netdev - Re: [PATCH net] r8169: work around short packet hw bug on RTL8125

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]

Date:   Thu, 5 Nov 2020 08:13:00 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Maxim Plotnikov <wgh@...lan.ru>
Subject: Re: [PATCH net] r8169: work around short packet hw bug on RTL8125

On Thu, 5 Nov 2020 11:31:48 +0100 Heiner Kallweit wrote:
> >> @@ -4125,7 +4133,7 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private *tp,
> >>  
> >>  		opts[1] |= transport_offset << TCPHO_SHIFT;
> >>  	} else {
> >> -		if (unlikely(rtl_test_hw_pad_bug(tp, skb)))
> >> +		if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
> >>  			return !eth_skb_pad(skb);
> >>  	}  
> > 
> > But looks like we may have another bug here - looks like this function
> > treas skb_cow_head() and eth_skb_pad() failures the same, but former
> > doesn't free the skb on error, while the latter does.
> >   
> Thanks for the hint, indeed we have an issue. The caller of
> rtl8169_tso_csum_v2() also frees the skb if false is returned, therefore
> we have a double free if eth_skb_pad() fails.
> 
> When checking eth_skb_pad() I saw that it uses kfree_skb() to free
> the skb on error. Kernel documentation say about ndo_start_xmit context:
> 
> Process with BHs disabled or BH (timer),
> will be called with interrupts disabled by netconsole.
> 
> Is it safe to use kfree_skb() if interrupts are disabled?
> I'm asking because dev_kfree_skb_any() uses the irq path if
> irqs_disabled() is true.

It is not, although in practice it only matters if skb has a socket
attached, and AFAIR netconsole doesn't, so no-one has ever seen the
WARN() trigger. But yes, I think it's best if we fix it.