From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [PATCH net] net: validate untrusted gso packets Date: Thu, 18 Jan 2018 17:33:27 +0800 Message-ID: References: <20180116202914.205914-1-willemdebruijn.kernel@gmail.com> <39e93ed0-3e2c-20bd-ebb1-deeb37940175@redhat.com> <28e14b63-5756-3384-ab16-f70911779f2a@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Network Development , David Miller , Eric Dumazet , Willem de Bruijn To: Willem de Bruijn Return-path: Received: from mx1.redhat.com ([209.132.183.28]:51934 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754617AbeARJdb (ORCPT ); Thu, 18 Jan 2018 04:33:31 -0500 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 2018年01月18日 13:09, Willem de Bruijn wrote: >>> Also, a packet can just as easily spoof an esp packet. >>> See also the discussion in the Link above. >> Then we probably need check there. > Adding a check in every gso handler that does not expect packets > with SKB_GSO_DODGY source seems backwards to me. > > A packet with gso_type SKB_GSO_TCPV4 should never be > delivered to an sctp handler, say. We must check before passing > to a handler whether the packet matches the callback. That's the case for trusted source. For dodgy source, we can't assume that. > >>> We can address this specific issue in segmentation by placing a check >>> analogous to skb_validate_dodgy_gso in the network layer. Something >>> like this (but with ECN option support): >>> >>> @@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff >>> *skb, >>> >>> skb_reset_transport_header(skb); >>> >>> + gso_type = skb_shinfo(skb)->gso_type; >>> + if (gso_type & SKB_GSO_DODGY) { >>> + switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) { >>> + case SKB_GSO_TCPV4: >>> + if (proto != IPPROTO_TCP) >>> + goto out; >>> + break; >>> + case SKB_GSO_UDP: >>> + if (proto != IPPROTO_UDP) >>> + goto out; >>> + break; >>> + default: >>> + goto out; >>> + } >>> + } >> This implements subset of function for codes which was removed by the commit >> I mentioned below. > No, as I explain above, it performs a different check. > >>>> [...] >>>> For performance reason. I think we should delay the check or segmentation >>>> as >>>> much as possible until it was really needed. >>> Going through segmentation is probably as expensive as flow dissector, >>> if not more so because of the indirect branches. >> I think we don't even need to care about this consider the evil packet >> should be rare. > How does frequency matter when a single packet can crash a host? I mean consider we had fix the crash, we don't care how expensive do we spot this. > >> And what you propose here is just a very small subset of the >> necessary checking, more comes at gso header checking. So even if we care >> performance, it only help for some specific case. > It also fixed the bug that Eric sent a separate patch for, as that did > not dissect as a valid TCP packet, either. I may miss something but how did this patch protects an evil thoff? >>> And now we have two >>> pieces of code that need to be hardened against malicious packets. >> To me fixing the exist path is a good balance between security and >> performance. >> >>> But I did overlook the guest to guest communication, with virtual devices >>> that can set NETIF_F_GSO_ROBUST and so do not enter the stack. That >>> means that one guest can send misrepresented packets to another. Is that >>> preferable to absorbing the cost of validation? >> The problem is that guests and hosts don't trust each other. Even if one >> packet has already been validated by one side, it must be validated again by >> another side when needed. Doing validation twice is meaningless in this >> case. > Ack, agreed that guests need to have defensive coding of themselves. > Then it's fine to pass packets to guests where the gso_type does not > match the contents. > >>> The current patch does not actually deprecate the path through the >>> segmentation layer. So it will indeed add overhead. We would have to >>> remove the DODGY logic in net-next. >> I don't get the point of removing this. >> >>> One additional possible optimization >>> is to switch to flow_keys_buf_dissector, which does not dissect as deeply. >>> I did that in an earlier patch; it should be sufficient for this purpose. >> I suspect the cost is still noticeable, and consider we may support kind of >> tunnel offload in the future. >> >> We should first assume the correctness of metadata provided by untrusted >> source and validate it before we really want to use it. Validate them during >> entry means we assume they are wrong, then there's no need for most of the >> fields of virtio-net header, > If you always need to use the data, then you always validate. In which case > you want to validate early, as there will be less vulnerable code before > validation. > > But I see what I think is your point: in guest to guest communication we > do not need to validate at all, so early validation adds unnecessary cost > for this important use case. That's a fair argument for validating later and > only when needed (i.e., a path without NETIF_F_GSO_ROBUST). Yes, and looking at Herbert's commit log for 576a30eb6453 ("[NET]: Added GSO header verification"). It was intended do the verification just before gso. Thanks