On Fri, Jan 19, 2018 at 3:19 AM, Jason Wang <jasow...@redhat.com> wrote: > > > On 2018年01月19日 08:53, Willem de Bruijn wrote: >>>>> >>>>> 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? >> >> Actually, it blocked that specific reproducer because the ip protocol >> did not match. > > > I see. > >> >> I think that __skb_flow_dissect_tcp should return a boolean, causing >> dissection return FLOW_DISSECT_RET_OUT_BAD if the tcph is bad. >> That would be needed to really catch it with flow dissection at the >> source. > > > Just sanitize transport to offset_hint (0) in the case of tun.
That is the current approach in skb_probe_transport_header, but it opens us up to parsing of garbage headers later in the stack when unconditionally reading tcp_hdrlen. The qdisc_pkt_len_init case is one such example. Seems better to leave transport header unset in such cases and qualify all access to the headers if DODGY. > It looks to > me flow dissector will return FLOW_DISSECT_RET_OUT_BAD too if it can't > recognize the protocol. We can't differ the real failure from unrecognized > protocol. (or change the return from bool to int). Unrecognized protocol should imply failure. virtio_net_hdr_to_skb accepts a whitelist of protocols, all of which the flow dissector can verify. Another argument for early validation: we cannot currently differentiate tunnel headers inserted by the tun/pf_packet/xen user from those generated by the stack if both are present. The kernel currently does not define tunnel VIRTIO_NET_HDR_GSO types, so should drop packets with tunnel headers. Tunnel gso handlers indeed do drop these if skb->encapsulation is not set. But if a packet travels through a tunnel device and the bit is set, at segmentation time we cannot distinguish trusted stack headers from possibly malformed user supplied ones.
