netdev - Re: [Patch net] igb: pass the correct maxlen for eth_get

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: <553821E4.5010901@gmail.com>
Date:	Wed, 22 Apr 2015 15:34:12 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Cong Wang <cwang@...pensource.com>
CC:	Cong Wang <xiyou.wangcong@...il.com>,
	netdev <netdev@...r.kernel.org>,
	intel-wired-lan@...ts.osuosl.org,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Subject: Re: [Patch net] igb: pass the correct maxlen for eth_get_headlen()

On 04/22/2015 02:56 PM, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 2:42 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On 04/22/2015 01:33 PM, Cong Wang wrote:
>>> First, make sure you don't miss the TSIP case right above:
>>>
>>> The frag starting pointer and its size are advanced by:
>>>
>>> skb_frag_size_sub(frag, IGB_TS_HDR_LEN);
>>> ...
>>> va += IGB_TS_HDR_LEN;
>>>
>>> even though we unlikely pull header longer than
>>> IGB_RX_HDR_LEN - IGB_TS_HDR_LEN either.
>> So I believe this is a possible bug, one heck of a corner case to get
>> into though.  It requires timestamp in packet, size 240 - 256, and a
>> malformed header.
>>
>> The proper fix would probably be to pull the timestamp out of the packet
>> before we add it to the frame.  I'll submit a patch to address this.
>>
>
> Huh? Doesn't my patch already fix this? skb_frag_size() is always
> up to date. Or you mean another different problem?

Your patch has other issues.  I am still NAKing your patch, but there is
an issue with igb that you have pointed out.  The proper fix would be to
deal with the timestamp before we add the page fragment to the skb.

>
>>> Second, the check you mentioned above is:
>>>
>>> if ((size <= IGB_RX_HDR_LEN) && !skb_is_nonlinear(skb))
>>>
>>> skb is nonlinear _after_ the first igb_add_rx_frag(), a second
>>> igb_add_rx_frag() is possible since igb_is_non_eop() could
>>> return true.
>> I'm not sure this part makes any sense.  We pull the data out of the
>> first fragment always.  If skb_is_nonlinear is set then we should have
>> at least 2K - 16B in the case of igb.  We will never have a second
>> fragment without at least 2K of data being given in the first.
> Apparently my igb knowledge isn't enough to verify this, I just did
> logical analysis.

The logic with igb is that if skb_is_nonlinear it means that a page has
already been added.  The way the hardware works is that it will break a
frame up into 2K segments until the entire frame is written.  So the
only way to get a second page fragment is if the first has used the
entire 2K buffer it was given.

- Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html