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]

Date:	Thu, 23 Apr 2015 11:40:18 -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/23/2015 11:06 AM, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 8:40 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On 04/22/2015 04:23 PM, Cong Wang wrote:
>>> On Wed, Apr 22, 2015 at 3:34 PM, Alexander Duyck
>>> <alexander.duyck@...il.com> wrote:
>>>> 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.
>>>>
>>> If the first frag is always 2K, then this is not a problem either.
>>> IGB_TS_HDR_LEN + IGB_RX_HDR_LEN < 2K.
>> The problem is skb->tail will get screwed up.
>>
> Why? We don't copy timestamp into skb header, instead it is saved
> in shared_info, why skb->tail matters here in TSIP case?

TSIP only matters if you have a network header with bad data in it that
indicates the size is actually larger than it actually is.

>> The first frag will be 2K in size only if there are multiple frags.  The
>
> Or it doesn't have frags at all since we just copy it to skb header, right?

That is moot since this code only gets called if the skb is nonlinear

>> problem in the TSIP case is that we will put the size reported by the
>> descriptor into the fragment, and then try to pull the size we
>
> That size is saved when adding the frag, in TSIP case we just sub it
> by IGB_TS_HDR_LEN, which seems correct.
>

Yes, the size is saved.  But with your solution we could pull the whole
fragment but not free it which isn't correct as we shouldn't be left
with any 0 sized fragments.

>> determined via eth_get_headlen.  So there is a window where that value
>> can be wrong and result in data_len going negative and tail being moved
>> beyond the end of the actual data.
> This sounds like a different problem if you are saying we should sub
> the size in the descriptor too. Therefore I don't see why your patch
> could replace mine.

Your patches sort-of fixed the problem, but they introduced other issues
in the process.

The thing I don't think you are getting is that the code was meant to be
mutually exclusive w/ the copy-break code.  Either the frag is less than
IGB_RX_HDR_SIZE in which case we copy-break and don't attached the
fragment, or we should be pulling the header and leaving at least 1 byte
in the fragment.  The problem with your solution is that you potentially
pull the entire fragment in, but you don't free it if the size drops to
0.  That is why in the other mail I told you the better solution for igb
would likely be to just pass IGB_RX_HDR_SIZE - IGB_TS_HDR_SIZE as that
way you end up with at least 1 byte left in the fragment after you pull
the header.

- 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