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: <CAHA+R7P=vT_aVJU+JP91Nrn+Xury=aGOfCmHPRU50CEk_EbzcA@mail.gmail.com>
Date:	Thu, 23 Apr 2015 12:14:25 -0700
From:	Cong Wang <cwang@...pensource.com>
To:	Alexander Duyck <alexander.duyck@...il.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 Thu, Apr 23, 2015 at 11:40 AM, Alexander Duyck
<alexander.duyck@...il.com> wrote:
> 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.


OK.

>
>>> 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.


Good point, then TSIP case should check frag_size == 0 case,
the rest handles 0-size case correctly, see below.

>
>>> 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


Only if it is a malformed packet, and it is still safe as long as we don't
run over the buffer size. Also I have a upper limit check for
IGB_RX_HDR_SIZE after eth_get_headlen().

> 0.  That is why in the other mail I told you the better solution for igb

I am sure eth_get_headlen() handles 0 case correctly, it simply returns 0.

> 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.
>

The code should already handle pull_len==0 correctly.
--
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