| Message ID | 1363333305-54398-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jason Wang <jasowang@redhat.com> Date: Fri, 15 Mar 2013 15:41:44 +0800 > Commit 1def9238d4aa2 (net_sched: more precise pkt_len computation) tries to do > precise packet len computation for GSO packets, but it does not check whether > the packets were from untrusted source. This is wrong since: we haven't done > header check before so both gso_segs and headers may not be correct. So this > patch just bypass the precise pkt_len computation for packet from untrusted > source (SKB_GSO_DODGY). > > Cc: Eric Dumazet <edumazet@google.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> I do not think this is appropriate or even necessary. All the user can do by reporting an incorrect header size or GSO segs is hurt himself, by making his traffic take more packet scheduler quota. When we do precise accounting, it increases, never decreases, the amount that a packet "costs" as far as the packet scheduler is concerned. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/18/2013 12:10 AM, David Miller wrote: > From: Jason Wang <jasowang@redhat.com> > Date: Fri, 15 Mar 2013 15:41:44 +0800 > >> Commit 1def9238d4aa2 (net_sched: more precise pkt_len computation) tries to do >> precise packet len computation for GSO packets, but it does not check whether >> the packets were from untrusted source. This is wrong since: we haven't done >> header check before so both gso_segs and headers may not be correct. So this >> patch just bypass the precise pkt_len computation for packet from untrusted >> source (SKB_GSO_DODGY). >> >> Cc: Eric Dumazet <edumazet@google.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> > I do not think this is appropriate or even necessary. > > All the user can do by reporting an incorrect header size or GSO segs > is hurt himself, by making his traffic take more packet scheduler > quota. I believe before doing header check for untrusted packets, the only thing we can trust is skb->len and that's we've used before 1def9238d4aa2. But after that, we're trying to use unchecked or meaningless value (e.g gso_segs were reset to zero in tun/macvtap/packet), and guest then can utilize this to result a very huge (-1U) pkt_len by filling evil value in the header. Can all kinds of packet scheduler survive this kinds of possible DOS? > > When we do precise accounting, it increases, never decreases, the > amount that a packet "costs" as far as the packet scheduler is > concerned. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-03-19 at 17:25 +0800, Jason Wang wrote: > I believe before doing header check for untrusted packets, the only > thing we can trust is skb->len and that's we've used before > 1def9238d4aa2. But after that, we're trying to use unchecked or > meaningless value (e.g gso_segs were reset to zero in > tun/macvtap/packet), and guest then can utilize this to result a very > huge (-1U) pkt_len by filling evil value in the header. Can all kinds of > packet scheduler survive this kinds of possible DOS? I would use the flow dissector to fix the transport header from all DODGY providers. Daniel Borkmann is working on a patch serie adding nhoff into flow_keys, and adding __skb_get_poff(const struct sk_buff *skb), for a BPF extension we talked about in Copenhagen / Netfilter Workshop. You could then set the transport header offset to the right value. (and drop evil packets before they go further in the stack) if (gso_packet(skb)) { u32 poff = __skb_get_poff(skb); if (!poff) { drop_evil_packet(skb); } else { skb_set_transport_header(skb, poff); ... } } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-03-19 at 05:10 -0700, Eric Dumazet wrote: > On Tue, 2013-03-19 at 17:25 +0800, Jason Wang wrote: > > > I believe before doing header check for untrusted packets, the only > > thing we can trust is skb->len and that's we've used before > > 1def9238d4aa2. But after that, we're trying to use unchecked or > > meaningless value (e.g gso_segs were reset to zero in > > tun/macvtap/packet), and guest then can utilize this to result a very > > huge (-1U) pkt_len by filling evil value in the header. Can all kinds of > > packet scheduler survive this kinds of possible DOS? > > I would use the flow dissector to fix the transport header from all > DODGY providers. > > Daniel Borkmann is working on a patch serie adding nhoff into flow_keys, > and adding __skb_get_poff(const struct sk_buff *skb), for a BPF > extension we talked about in Copenhagen / Netfilter Workshop. > > You could then set the transport header offset to the right value. > > (and drop evil packets before they go further in the stack) > > if (gso_packet(skb)) { > u32 poff = __skb_get_poff(skb); > > if (!poff) { > drop_evil_packet(skb); > } else { > skb_set_transport_header(skb, poff); > ... > } > } Oh well, no need to use __skb_get_poff() but plain skb_flow_dissect() (once patched to include thoff in struct flow_keys) struct flow_keys keys; if (!skb_flow_dissect(skb, &keys)) goto drop; if ((gso_type & (SKB_GSO_TCPV4|SKB_GSO_TCPV6)) && keys.ip_proto != IP_PROTO_TCP) goto drop; skb_set_transport_header(skb, keys.thoff); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/19/2013 08:58 PM, Eric Dumazet wrote: > On Tue, 2013-03-19 at 05:10 -0700, Eric Dumazet wrote: >> On Tue, 2013-03-19 at 17:25 +0800, Jason Wang wrote: >> >>> I believe before doing header check for untrusted packets, the only >>> thing we can trust is skb->len and that's we've used before >>> 1def9238d4aa2. But after that, we're trying to use unchecked or >>> meaningless value (e.g gso_segs were reset to zero in >>> tun/macvtap/packet), and guest then can utilize this to result a very >>> huge (-1U) pkt_len by filling evil value in the header. Can all kinds of >>> packet scheduler survive this kinds of possible DOS? >> I would use the flow dissector to fix the transport header from all >> DODGY providers. >> >> Daniel Borkmann is working on a patch serie adding nhoff into flow_keys, >> and adding __skb_get_poff(const struct sk_buff *skb), for a BPF >> extension we talked about in Copenhagen / Netfilter Workshop. >> >> You could then set the transport header offset to the right value. >> >> (and drop evil packets before they go further in the stack) >> >> if (gso_packet(skb)) { >> u32 poff = __skb_get_poff(skb); >> >> if (!poff) { >> drop_evil_packet(skb); >> } else { >> skb_set_transport_header(skb, poff); >> ... >> } >> } > > Oh well, no need to use __skb_get_poff() but plain skb_flow_dissect() > (once patched to include thoff in struct flow_keys) > > struct flow_keys keys; > > if (!skb_flow_dissect(skb, &keys)) > goto drop; > > if ((gso_type & (SKB_GSO_TCPV4|SKB_GSO_TCPV6)) && > keys.ip_proto != IP_PROTO_TCP) > goto drop; > > skb_set_transport_header(skb, keys.thoff); I was consider a just skb_reset_transport_header() here. Consider the transport header maybe checked and reset during header check for packets of gso or partial checksum. And bypass precise pkt len computation. Some problems with skb_flow_dissect(): - it can only recognizes a subset of all ethernet protocols. The may blocks guest who may want to use other protocol such as IPX. - almost no check in the validity of the L4 protocol header which may be used by qdisc_pkt_len_init(), which may still give a chance to evil guest to use - gso_segs were untouched (still zero) Another method is doing header check here which needs more work. > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-03-20 at 14:19 +0800, Jason Wang wrote: > I was consider a just skb_reset_transport_header() here. Consider the > transport header maybe checked and reset during header check for packets > of gso or partial checksum. And bypass precise pkt len computation. > > Some problems with skb_flow_dissect(): > > - it can only recognizes a subset of all ethernet protocols. The may > blocks guest who may want to use other protocol such as IPX. Oh yes IPX > - almost no check in the validity of the L4 protocol header which may be > used by qdisc_pkt_len_init(), which may still give a chance to evil > guest to use > - gso_segs were untouched (still zero) > > Another method is doing header check here which needs more work. Most uses are caught by the helper. So in the case dissection fails, just reset transport header instead of dropping as I suggested. Otherwise, set the transport header to the right value -> precise qdisc pkt lengths. Instead of pretending DODGY packets have no headers, at least try to do the right thing. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/dev.c b/net/core/dev.c index 90cee5b..480114d 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2586,7 +2586,7 @@ static void qdisc_pkt_len_init(struct sk_buff *skb) /* To get more precise estimation of bytes sent on wire, * we add to pkt_len the headers size of all segments */ - if (shinfo->gso_size) { + if (shinfo->gso_size && !(shinfo->gso_type & SKB_GSO_DODGY)) { unsigned int hdr_len; /* mac layer + network layer */
Commit 1def9238d4aa2 (net_sched: more precise pkt_len computation) tries to do precise packet len computation for GSO packets, but it does not check whether the packets were from untrusted source. This is wrong since: we haven't done header check before so both gso_segs and headers may not be correct. So this patch just bypass the precise pkt_len computation for packet from untrusted source (SKB_GSO_DODGY). Cc: Eric Dumazet <edumazet@google.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- net/core/dev.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)